Double checked locking Article

Question

I was reading this article about "Double-Checked locking" and out of the main topic of the article I was wondering why at some point of the article the author uses the next Idiom:

Listing 7. Attempting to solve the out-of-order write problem

public static Singleton getInstance()  
{
    if (instance == null)
    {
        synchronized(Singleton.class) {      //1
            Singleton inst = instance;         //2
            if (inst == null)
            {
                synchronized(Singleton.class) {  //3
                    inst = new Singleton();        //4
                }
                instance = inst;                 //5
            }
        }
    }
    return instance;
}

And my question is: Is there any reason to synchronize twice some code with the same lock? Have this any purpose it?

Many thanks in advance.

Answer 1

The point of locking twice was to attempt to prevent out-of-order writes. The memory model specifies where reorderings can occur, partly in terms of locks. The lock ensures that no writes (including any within the singleton constructor) appear to happen after the "instance = inst;" line.

However, to go deeper into the subject I'd recommend Bill Pugh's article. And then never attempt it :)

Answer 2

The article refers to the pre-5.0 Java memory model (JMM). Under that model leaving a synchronised block forced writes out to main memory. So it appears to be an attempt to make sure that the Singleton object is pushed out before the reference to it. However, it doesn't quite work because the write to instance can be moved up into the block - the roach motel.

However, the pre-5.0 model was never correctly implemented. 1.4 should follow the 5.0 model. Classes are initialised lazily, so you might as well just write

public static final Singleton instance = new Singleton();

Or better, don't use singletons for they are evil.

Answer 3

Jon Skeet is right: read Bill Pugh's article. The idiom that Hans uses is the precise form that won't work, and should not be used.

This is unsafe:

private static Singleton instance;

public static Singleton getInstance() {
  if (instance == null) {
    synchronized(Singleton.class) {
      if (instance == null) {
        instance = new Singleton();
      }
    }
  }
  return instance;
}

This is also unsafe:

public static Singleton getInstance()  
{
    if (instance == null)
    {
        synchronized(Singleton.class) {      //1
            Singleton inst = instance;         //2
            if (inst == null)
            {
                synchronized(Singleton.class) {  //3
                    inst = new Singleton();        //4
                }
                instance = inst;                 //5
            }
        }
    }
    return instance;
}

Don't do either of them, ever.

Instead, synchronise the whole method:

    public static synchronized Singleton getInstance() {
      if (instance == null) {
        instance = new Singleton();
      }
      return instance;
    }

Unless you're retrieving this object a zillion times a second the performance hit, in real terms, is negligible.

Answer 4

I cover a bunch of this here:

http://tech.puredanger.com/2007/06/15/double-checked-locking/