Why is double-checked locking broken in Java?

Question

This question relates to behaviour of old Java versions and old implementations of the double checked locking algorithm

Newer implementations use volatile and rely on slightly changed volatile semantics, so they are not broken.

---

It's stated that fields assignment is always atomic except for fields of long or double.

But, when I read an explaination of why double-check locking is broken, it's said that the problem is in assignment operation:

// Broken multithreaded version
// "Double-Checked Locking" idiom
class Foo {
    private Helper helper = null;
    public Helper getHelper() {
        if (helper == null) {
            synchronized(this) {
                if (helper == null) {
                    helper = new Helper();
                }
            }
        }
        return helper;
    }

    // other functions and members...
}

1. Thread A notices that the value is not initialized, so it obtains the lock and begins to initialize the value.
2. Due to the semantics of some programming languages, the code generated by the compiler is allowed to update the shared variable to point to a partially constructed object before A has finished performing the initialization.
3. Thread B notices that the shared variable has been initialized (or so it appears), and returns its value. Because thread B believes the value is already initialized, it does not acquire the lock. If B uses the object before all of the initialization done by A is seen by B (either because A has not finished initializing it or because some of the initialized values in the object have not yet percolated to the memory B uses (cache coherence)), the program will likely crash.
   (from http://en.wikipedia.org/wiki/Double-checked_locking).

When is it possible? Is it possible that on 64-bit JVM assignment operation isn't atomic? If no then whether "double-checked locking" is really broken?

Answer 1

The problem is not atomicity, it's ordering. The JVM is allowed to reorder instructions in order to improve performance, as long as happens-before is not violated. Therefore, the runtime could theoretically schedule the instruction that updates helper before all instructions from the constructor of class Helper have executed.

Answer 2

I'm sorry this might be a bit irrelevant to the question, I'm just curious. In this case wouldn't it better to acquire the lock before the assignment and/or returning the value? Like:

private Lock mLock = new ReentrantLock();
private Helper mHelper = null;

private Helper getHelper() {
    mLock.lock();
    try {
        if (mHelper == null) {
            mHelper = new Helper();
        }
        return mHelper;
    }
    finally {
        mLock.unlock();
    }
}

Or is there any advantage of using the double-checked locking?

Answer 3

Read this article: http://www.javaworld.com/jw-02-2001/jw-0209-double.html Even if you did not understand all details (like me) just believe that this nice trick does not work.

Answer 4

The assignment of the reference is atomic, but the construction is not! So as stated in the explanation, supposing thread B wants to use the singleton before Thread A has fully constructed it, it cannot create a new instance because the reference is not null, so it just returns the partially constructed object.

If you do not ensure that publishing the shared reference happens before another thread loads that shared reference, then the write of the reference to the new object can be reordered with the writes to its fields. In that case, another thread could see an up-to-date value for the object reference but out of date values for some or all of the object's state - a partially constructed object. -- Brian Goetz: Java Concurrency in Practice

Since the initial check for null is not synchronized there is no publication and this reordering is possible.

Answer 5

Double checked locking in java has a variety of problems:

http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Answer 6

/*Then the following should work.
  Remember: getHelper() is usually called many times, it is BAD 
  to call synchronized() every time for such a trivial thing!
*/
class Foo {

private Helper helper = null;
private Boolean isHelperInstantiated;
public Helper getHelper() {
    if (!isHelperInstantiated) {
        synchronized(this) {
            if (helper == null) {
                helper = new Helper();
                isHelperInstantiated = true;
            }
        }
    }
    return helper;
}

// other functions and members...
}