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.

@Aishwar, your should probably read the Wikipedia page you link to in more details, in particular what it says about `volatile`: use it (or don't use DCL). — Bruno, Dec 06 '11 at 23:06
It's kind of pointless given that the method always acquires the lock. — Tom Hawtin - tackline, Dec 06 '11 at 23:18
@KirkWoll My understanding may be wrong here. But I imagined this is what happens. There is a lock on `this` while assigning `helper`. And there is another lock at the beginning of the `getHelper` function on `this`. So if `getHelper` is called on Thread 1 when an assignment is going on in Thread 2, the first `synchronized` block holds the execution of Thread 1 till the assignment on Thread 2 is completed. So the value of helper is never accessed while it is being set. — Aishwar, Dec 06 '11 at 23:20
@TomHawtin-tackline I agree this may not be efficient, but I have modeled some existing code and am just trying to analyze if this is affected by the `double-checked-locking` issue. — Aishwar, Dec 06 '11 at 23:22
there is NO double-check, the whole idea is skipping the contention point, i.e. acquiring the lock. In that case @Tom is absolutely correct — bestsss, Dec 07 '11 at 09:43
This stuff is hard to get right and easy to mess up. You can use Guava's `Suppliers.memoize(Supplier)`, which implements this for you! — Ray, Dec 07 '11 at 13:35

Rich O'Kelly · Answer 1 · 2011-12-07T09:32:51.353

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.

You should assign helper to a local variable. – bestsss Dec 07 '11 at 09:43 — bestsss, Dec 07 '11 at 09:43

score 3 · Accepted Answer · answered Dec 06 '11 at 23:03

3

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.

answered Dec 06 '11 at 23:03

yshavit

42,327
7
87
124

Thanks. This answers my question. It explains why the above code is not double-checked locking. – Aishwar Dec 08 '11 at 22:33

score 0 · Answer 3 · answered Dec 06 '11 at 23:05

0

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.

answered Dec 06 '11 at 23:05

Viruzzo

3,025
13
13

@Viruzzo My understanding may be wrong here. But I imagined this is what happens. There is a lock on this while assigning helper. And there is another lock at the beginning of the getHelper function on this. So if getHelper is called on Thread 1 when an assignment is going on in Thread 2, the first synchronized block holds the execution of Thread 1 till the assignment on Thread 2 is completed. So the value of helper is never accessed while it is being set. – Aishwar Dec 06 '11 at 23:24

Is this code Double Checked Locking safe?

3 Answers3