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

The whole point of double-checked locking is that the fast path (when you don't need to instantiate the object) isn't synchronized. So what you have isn't double-checked locking.

You need to get rid of the first synchronized block in order to get a broken double-checked lock. Then you need to make helper volatile to fix it.

Answer 2

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.

Answer 3

This part is a standard double-checked locking:

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

The first part is a useless assignment that does nothing to the double-checked locking part: if helper is null then it is executed anyway, if it isn't it wouldn't be executed anyway. It's completely ineffective.