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

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
– Stephen C, Commented Dec 10, 2009 at 5:33
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
– Kevin Montrose, Commented Dec 10, 2009 at 5:40
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. — Cornelius Dol
– Cornelius Dol, Commented Dec 10, 2009 at 6:16

erickson · Accepted Answer · 2009-12-10 06:41:29Z

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.

answered Dec 10, 2009 at 6:41

erickson

271k59 gold badges406 silver badges502 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Thorbjørn Ravn Andersen Over a year ago

Do you have a link to a good tutorial for lazy initialization patterns in the latest versions of Java?

erickson Over a year ago

Check out the "Initialization on Demand Holder" idiom: en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom

blackanchorage Over a year ago

I like IODH too - simple, lazy initialization, fast and no problems with synchronization.

Thilo · Accepted Answer · 2009-12-10 05:28:54Z

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.

answered Dec 10, 2009 at 5:28

Thilo

264k107 gold badges527 silver badges674 bronze badges

Comments

Andrew Lygin · Accepted Answer · 2009-12-10 09:00:08Z

1

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

Thread1 checks if successfully and stops on synchronization line
Thread2 checks if successfully and stops on synchronization code.
Thread3 checks if successfully and stops on synchronization code.
Thread1 falls into sync block, creates helper and leaves this block.
Thread2 falls into sync block, increment timeSafelyGotten and leaves this block.
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) {
        ....
    }
}

edited Dec 10, 2009 at 9:00

answered Dec 10, 2009 at 5:35

Andrew Lygin

6,2271 gold badge35 silver badges38 bronze badges

2 Comments

Caleb James DeLisle Over a year ago

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.

Andrew Lygin Over a year ago

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.

Collectives™ on Stack Overflow

Java double checked locking by forcing synchronization twice, Workable?

3 Answers 3

3 Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related