Java double checked locking by forcing synchronization twice, Workable?

Question

I've read all about how double checked locking fixes never work and I don't like lazy initialization, but it would be nice to be able to fix legacy code and such a problem is too enticing not to try to solve.

Here is my example: private int timesSafelyGotten = 0; private Helper helper = null;

public getHelper()
{
    if (timesSafelyGotten < 1) {
        synchronized (this) {
            if (helper == null) {
                helper = new Helper();
            } else {
                timesSafelyGotten++;
            }
        }
    }
    return helper;
}

This way the synchronized code must run once to create the helper and once when it is gotten for the first time so theoretically timesSafelyGotten cannot be incremented until after the synchronized code which created the helper has released the lock and the helper must be finished initializing.

I see no problems, but it is so simple it seems too good to be true, what do you think?

Caleb James DeLisle

Answer 1

If you are using JDK5+, use java.util.concurrent, in your case probably AtomicInteger.

These utilities are provided specifically because no one can be expected to understand the low-level thread synchronization primitives well enough to make them work properly.

Answer 2

That's not good. You can get timeSafelyGotten > 1. Example:

1. Thread1 checks if successfully and stops on synchronization line
2. Thread2 checks if successfully and stops on synchronization code.
3. Thread3 checks if successfully and stops on synchronization code.
4. Thread1 falls into sync block, creates helper and leaves this block.
5. Thread2 falls into sync block, increment timeSafelyGotten and leaves this block.
6. Thread3 falls into sync block, increment timeSafelyGotten and leaves this block.

So timeSafelyGotten = 2.

You should add one more check:

if (helper == null) {
    helper = new Helper();
} else if (timesSafelyGotten < 1) {
    timesSafelyGotten++;
}

or move sync upper:

synchronized(this) {
   if (timeSafelyGotten < 1) {
       ...
   }
}

The first way is better because it doesn't use sync every time.

One more hint: Don't use synchronize(this) because somebody can use your object for synchronization too. Use special private object for internal synchronization:

classs MyClass {
    private Object syncRoot = new Object();

    ...
    synchronized(syncRoot) {
        ....
    }
}

Answer 3

Without a memory barrier (synchronized, volatile, or equivalent from java.util.concurrent), a thread may see actions of another thread occur in a different order than they appear in source code.

Since there's no memory barrier on the read of timesSafelyGotten, it can appear to another thread that timesSafelyGotten is incremented before helper is assigned. That would result in returning null from the method.

In practice, this might work on many architectures during your unit tests. But it's not correct and will eventually fail somewhere.

Double-checked locking does work now, but it's tricky to implement correctly and fairly expensive. There are patterns for lazy initialization that are less fragile, more readable, and don't require anything exotic.