Double checked locking pattern: Broken or not?

Why is the pattern considered broken? It looks fine to me? Any ideas?

public static Singleton getInst() {
    if (instace == null) createInst();
    return instace;
}

private static s开发者_StackOverflow社区ynchronized createInst() {
     if (instace == null) {
         instace = new Singleton(); 
     }
}

It looks okay at first glance, but this technique has many subtle problems and should usually be avoided. For example, consider the following sequence of events:

1. Thread A notices that the value is not initialized, so it obtains the lock and begins to initialize the value.
2. 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 the program will likely crash.

You could avoid this by using the "volatile" keyword to handle your singleton instances correctly

The whole discussion is a huge, endless waste of brain time. 99.9% of the time, singletons do not have any significant setup costs and there is no reason whatsoever for contrived setups to achieve non-synchronized guaranteed-lazy loading.

This is how you write a Singleton in Java:

public class Singleton{
    private Singleton instance = new Singleton();
    private Singleton(){ ... }
    public Singleton getInstance(){ return instance; }
}

Better yet, make it an enum:

public enum Singleton{
    INSTANCE;
    private Singleton(){ ... }
}

I don't know if it is broken, however it is not really the most efficient solution due to synchronizing which is rather expensive. A better approach would be using the 'Initialization On Demand Holder Idiom', which loads your singleton in memory the first time it is demanded, as the name implies, thus lazy loading. The biggest benefit you get with this idiom is that you don't need to synchronize because the JLS ensures that class loading is serial.

Detailed wikipedia entry on the subject: http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom

Another thing to keep in mind is that since dependency injection frameworks such as Spring and Guice have emerged, class instances are being created created and managed by these containers and they will provide you with a Singleton if desired, so it is not worth breaking your head over it, unless you want to learn to idea behind the pattern, which is useful. Also note that singletons provided by these IOC containers are singletons per container instance, but typically you'll have one IOC container per application, so it doesn't become an issue.

The problem is the following: Your JVM may reorder your code and fields are not always the same for different threads. Have a look at this: http://www.ibm.com/developerworks/java/library/j-dcl.html. The use of the volatile keyword should fix this, but its broken before java 1.5.

Most of the time single checked locking is more than fast enough, try this:

// single checked locking: working implementation, but slower because it syncs all the time
public static synchronized Singleton getInst() {
    if (instance == null) 
        instance = new Singleton();
    return instance;
}

Also have a look at effective java, where you will find a great chapter about this topic.

To sum this up: Dont do double checked locking, there are better idoms.

Initialization On Demand Holder Idiom, yup that's it:

public final class SingletonBean{

    public static SingletonBean getInstance(){
        return InstanceHolder.INSTANCE;
    }

    private SingletonBean(){}

    private static final class InstanceHolder{
        public static final SingletonBean INSTANCE = new SingletonBean();
    }

}

Although Joshua Bloch also recommends the Enum singleton pattern in Effective Java Chapter 2, Item 3:

// Enum singleton - the prefered approach
public enum Elvis{
    INSTANCE;
    public void leaveTheBuilding(){ ... }
}

This does not answer your question (others already have done), but I want to tell you my experience with singletons/lazy initialized objects:

We had a couple of singletons in our code. Once we had to add a constructor parameter to one singleton and had a serious problem, because the constructor of this singleton was invoked on the getter. There were only following possible solutions:

• provide a static getter (or another singleton) for the object which was needed to initialize this singleton,
• pass the object to initialize the singleton as parameter for the getter or
• get rid of the singleton by passing instances around.

Finally, the last option was the way to go. Now we initialize all objects at application start and pass required instances around (maybe as a small interface). We have not regretted this decision, because

• the dependencies of a piece of code is crystal clear,
• we can test our code much easier by providing dummy implementations of the required objects.

Most of the answers here are correct about why it is broken, but are incorrect or suggesting dubious strategies for a solution.

If you really, really must use a singleton (which in most cases you should not, as it destroys testability, combines logic about how to construct a class with the class's behavior, litters the classes that use the singleton with knowledge of how to get one, and leads to more brittle code) and are concerned about synchronization, the correct solution is to use a static initializer to instantiate the instance.

private static Singleton instance = createInst();

public static Singleton getInst() {
    return instance ;
}

private static synchronized createInst() {
    return new Singleton(); 
}

The Java language specification guarantees that static initiailzers will be run just once, when a class is loaded for the first time, and in a guaranteed thread-safe manner.