3

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

Georg Fritzsche
  • 97,545
  • 26
  • 194
  • 236
Caleb James DeLisle
  • 31
  • 2
  • 6
    If you read (and understand) one of the detailed explanations of why double checked locking is broken, you'll understand why your code is also broken. – Stephen C Dec 10 '09 at 05:33
  • 1
    Its also worth noting that with Java 5 and volatile you can make unbroken double checked locking. Java 4 and earlier your statement is indeed true. – Kevin Montrose Dec 10 '09 at 05:40
  • 2
    It's also worth noting that with J5 and greatly improved performance for uncontended locks DCL is almost always a premature optimization that is not worth the risk. – Lawrence Dol Dec 10 '09 at 06:16

3 Answers3

4

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.

erickson
  • 265,237
  • 58
  • 395
  • 493
2

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.

Thilo
  • 257,207
  • 101
  • 511
  • 656
1

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) {
        ....
    }
}
Andrew Lygin
  • 6,077
  • 1
  • 32
  • 37
  • Thank you for the quick reply, are not threads 2 and 3 blocked out of the synchronized code (at step 5) until helper is finished initializing? If so then it works doesn't it? Also I like synchronized(syncRoot) idea. – Caleb James DeLisle Dec 10 '09 at 06:34
  • Yes, they are blocked, but they are blocked after the first check and after unblocking they a fall into sync block. I've rewritten the example to make it more clear. – Andrew Lygin Dec 10 '09 at 09:03