Is this code Double Checked Locking safe?

Question

I am looking at some code in our app that I think may be encountering a case of "Double-checked locking". I have written some sample code that is similar to what we do.

Can anyone see how this can be experiencing double-checked locking? Or is this safe?

class Foo {
    private Helper helper = null;
    public Helper getHelper() {
        Helper result;
        synchronized(this) {
            result = helper;
        }

        if (helper == null) {
            synchronized(this) {
                if (helper == null) {
                    helper = new Helper();
                }
            }
        }
        return helper;
    }
}

Base code borrowed from wiki.

Answer 1

It's unnecessarily complex, the simplest 'safe' way to do DCL is like so:

class Foo {
  private volatile Helper helper = null;
  private final Object mutex = new Object(); 
  public Helper getHelper() {
    if (helper == null) {
        synchronized(mutex) {
            if (helper == null) {
                helper = new Helper();
            }
        }
    }
    return helper;
  }
}

The key points here being:

• In the 'happy' case we expect helper to already be assigned, so if it is we can just return it without having to enter a synchronized block.
• Helper is marked as volatile to let the compiler know that helper can be read from / written to by any thread at any time and it's important that read / writes are not re-ordered.
• The synchronized block uses a private final variable to synchronize on to avoid a potential performance hit in the case of another area of code synchronizing on the this instance.