Java double checked locking

Question

I happened upon an article recently discussing the double checked locking pattern in Java and its pitfalls and now I'm wondering if a variant of that pattern that I've been using for years now is subject to any issues.

I've looked at many posts and articles on the subject and understand the potential issues with getting a reference to a partially constructed object, and as far as I can tell, I don't think my implementation is subject to these issues. Are there any issues with the following pattern?

And, if not, why don't people use it? I've never seen it recommended in any of the discussion I've seen around this issue.

public class Test {
    private static Test instance;
    private static boolean initialized = false;

    public static Test getInstance() {
        if (!initialized) {
            synchronized (Test.class) {
                if (!initialized) {
                    instance = new Test();
                    initialized = true;
                }
            }
        }
        return instance;
    }
}

Answer 1

Double checked locking is indeed broken, and the solution to the problem is actually simpler to implement code-wise than this idiom - just use a static initializer.

public class Test {
    private static final Test instance = createInstance();

    private static Test createInstance() {
        // construction logic goes here...
        return new Test();
    }

    public static Test getInstance() {
        return instance;
    }
}

A static initializer is guaranteed to be executed the first time that the JVM loads the class, and before the class reference can be returned to any thread - making it inherently threadsafe.

Answer 2

The DCL problem is broken, even though it seems to works on many VMs. There is a nice writeup about the problem here http://www.javaworld.com/article/2075306/java-concurrency/can-double-checked-locking-be-fixed-.html.

multithreading and memory coherency are more complicated subjects than they might appear. [...] You can ignore all of this complexity if you just use the tool that Java provides for exactly this purpose -- synchronization. If you synchronize every access to a variable that might have been written, or could be read by, another thread, you will have no memory coherency problems.

The only way to solve this problem properly is to avoid lazy initialization (do it eagerly) or to single check inside a synchronized block. The use of the boolean initialized is equivalent to a null check on the reference itself. A second thread may see initialized being true but instance might still be null or partially initialized.

Answer 3

I've been investigating about the double checked locking idiom and from what I understood, your code could lead to the problem of reading a partially constructed instance UNLESS your Test class is immutable:

The Java Memory Model offers a special guarantee of initialization safety for sharing immutable objects.

They can be safely accessed even when synchronization is not used to publish the object reference.

(Quotations from the very advisable book Java Concurrency in Practice)

So in that case, the double checked locking idiom would work.

But, if that is not the case, observe that you are returning the variable instance without synchronization, so the instance variable may not be completely constructed (you would see the default values of the attributes instead of the values provided in the constructor).

The boolean variable doesn't add anything to avoid the problem, because it may be set to true before the Test class is initialized (the synchronized keyword doesn't avoid reordering completely, some sencences may change the order). There is no happens-before rule in the Java Memory Model to guarantee that.

And making the boolean volatile wouldn't add anything either, because 32 bits variables are created atomically in Java. The double checked locking idiom would work with them as well.

Since Java 5, you can fix that problem declaring the instance variable as volatile.

You can read more about the double checked idiom in this very interesting article.

Finally, some recommendations I've read:

• Consider if you should use the singleton pattern. It is considered an anti-pattern by many people. Dependency Injection is preferred where possible. Check this.

• Consider carefully if the double checked locking optimization is really necessary before implementing it, because in most cases, that wouldn't be worth the effort. Also, consider constructing the Test class in the static field, because lazy loading is only useful when constructing a class takes a lot of resources and in most of the times, it is not the case.

If you still need to perform this optimization, check this link which provides some alternatives for achieving a similar effect to what you are trying.

Answer 4

Double-checked Locking is the anti-pattern.

Lazy Initialization Holder Class is the pattern you should be looking at.

Despite so many other answers, I figured I should answer because there still isn't one simple answer that says why DCL is broken in many contexts, why it is unnecessary and what you should do instead. So I'll use a quote from Goetz: Java Concurrency In Practice which for me provides the most succint explanation in its final chapter on the Java Memory Model.

It's about Safe Publication of variables:

The real problem with DCL is the assumption that the worst thing that can happen when reading a shared object reference without synchronization is to erroneously see a stale value (in this case, null ); in that case the DCL idiom compensates for this risk by trying again with the lock held. But the worst case is actually considerably worse—it is possible to see a current value of the reference but stale values for the object's state, meaning that the object could be seen to be in an invalid or incorrect state.

Subsequent changes in the JMM (Java 5.0 and later) have enabled DCL to work if resource is made volatile , and the performance impact of this is small since volatile reads are usually only slightly more expensive than nonvolatile reads.

However, this is an idiom whose utility has largely passed—the forces that motivated it (slow uncontended synchronization, slow JVM startup) are no longer in play, making it less effective as an optimization. The lazy initialization holder idiom offers the same benefits and is easier to understand.

Listing 16.6. Lazy Initialization Holder Class Idiom.

public class ResourceFactory
    private static class ResourceHolder {
        public static Resource resource = new Resource();
    }

    public static Resource getResource() {
        return ResourceHolder.resource;
    }
}

That's the way to do it.

Answer 5

This is the reason why double checked locking is broken.

Synchronize guarantees, that only one thread can enter a block of code. But it doesn't guarantee, that variables modifications done within synchronized section will be visible to other threads. Only the threads that enters the synchronized block is guaranteed to see the changes. This is the reason why double checked locking is broken - it is not synchronized on the reader's side. The reading thread may see, that the singleton is not null, but singleton data may not be fully initialized (visible).

Ordering is provided by volatile. volatile guarantees ordering, for instance write to volatile singleton static field guarantees that writes to the singleton object will be finished before the write to volatile static field. It doesn't prevent creating singleton of two objects, this is provided by synchronize.

Class final static fields doesn't need to be volatile. In Java, the JVM takes care of this problem.

See my post, an answer to Singleton pattern and broken double checked locking in a real-world Java application, illustrating an example of a singleton with respect to double-checked locking that looks clever but is broken.

Answer 6

That would work if initialized was volatile. Just as with synchronized the interesting effects of volatile are not really so much to do with the reference as what we can say about other data. Setting up of the instance field and the Test object is forced to happen-before the write to initialized. When using the cached value through the short circuit, the initialize read happens-before reading of instance and objects reached through the reference. There is no significant difference in having a separate initialized flag (other than it causes even more complexity in the code).

(The rules for final fields in constructors for unsafe publication are a little different.)

However, you should rarely see the bug in this case. The chances of getting into trouble when using for the first time is minimal, and it is a non-repeated race.

The code is over-complicated. You could just write it as:

private static final Test instance = new Test();

public static Test getInstance() {
    return instance;
}