Double Checked Locking Is Broken

(See the bottom and CppDoubleCheckLock for C++ reference; most of this page discusses Java).

In short, the following code cannot be guaranteed to work:

	private Singleton() {
	}

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

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

	public static Singleton getInstance() {
		if (_theInstance == null) {
		_theInstance = this.getInstanceSlow();
		}
		return _theInstance;
	}

	private static synchronized Singleton getInstanceSlow() {
		if (_theInstance == null) {
		_theInstance = new Singleton();
		}
		return _theInstance;
	}

This is entirely equivalent to the inline version, and fails for the same reasons.

(Maybe we need a instance-level (non-static) method, to ensure that the call won't be inlined as 'final?')

How would that work, given that there is no instance when we first call 'getInstance'?

1. If you DO NOT NEED lazy initialization (and you often do not), then simply use a static member and initialize it right there (or in a static block). The class loader guarantees that only one thread will perform any class's static initialization. By the way, if the only accessible static field or method is getInstance() and no code directly accesses Singleton.class or invokes Class.forName("Singleton"), you essentially get lazy initialization for free thanks to the class loader.

	public class Singleton {
		private static final Singleton instance = new Singleton();

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

	public class Singleton {
		private static final Singleton instance;

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

		return instance;
		}
	}

	public class Singleton {
		private static class Instance {
		static final Singleton value = new Singleton();
		}

		public static Singleton getInstance() {
		return Instance.value;
		}
	}

The Java Language Standard states that class loaders can prefetch or otherwise group the loading of classes. So, no the behavior in option 3 above is not guaranteed. However, it generally holds true since most class loaders do not prefetch classes. -- ChuckMcCorvey

I'd like to add option 4) (slightly tongue in cheek).

4) Use a synchronized method and cache the result of the call to getInstance () in the code that is called very frequently - like you would for any performance critical section where the semantics of some function called allow you to cache the results.

-- AndySchneider

Consider the following code:

	public class Singleton {
	private static final Singleton instance = new Singleton();
	private boolean initialized = false;
	private Hashtable lotOfData = null;

	public static Singleton getInstance() {
	 if (!instance.isInitialized)
		instance.initialize(...);
	 return instance;
	}
	private Singleton() {
	 // no-operation
	}

	protected boolean isInitialized() {
	 return initialized;
	}

	protected synchronized void initialize(....) {
	 if (initialized)
		return;

	 lotOfData = new Hashtable();
	 lotOfData.add(....);
	 // Further initialization
	 initialized = true;
	}
	}

-- Anurag Wahi

It's not working, because you are assuming that initialized=true; will happen at the end of the initialize() function, after all the initialization has been done. In fact it is not unlikely for the processor to rearrange it exactly after if (initialized) return; because locally loks like a very good optimization if lotOfData.add(...) is inlined. Thus another thread might pick the (initialized == true) condition before the real initialization is executed.

• Maybe this would work if the initialized is set inside code that cannot be shuffled (optimized). Suppose initialized = true is done in a separate method doneInitializing() like this:

	protected synchronized void initialize(....) {
	 if (initialized)
		return;

	 lotOfData = new Hashtable();
	 lotOfData.add(....);
	 // Further initialization

	 doneInitializing();
	}

	protected void doneInitializing() {
	 initialized = true;
	}

• The compiler / JVM can rearrange code within synchronized blocks for optimisation.
• The compiler / JVM can move code into a synchronized block for optimisation.
• The compiler / JVM cannot move code out of a synchronized block.
• Threads can store local copies of variables that are not declared volatile or protected by a synchronized block so may not see changes made by other threads. (And volatile has a known bug - it doesn't appear to actually do anything).

-- DarrenHobbs

Double-checked locking: Clever, but broken http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html

Java Tip 67: Lazy instantiation http://www.javaworld.com/javaworld/javatips/jw-javatip67.html

Not every attempt at shared-state concurrency means that ACID semantics is needed (nor a database is required). Double-checked locking is typically an initialization pattern, used to guard a one-shot monotonic state transition (from the "not initialized" state to the "initialized" state), and often the thing being guarded is not business data. And it's almost never persistent, for that matter.'

• What is being sought is Atomic; but none of Consistent, Isolated, or Durable need necessarily apply.

The reason behind DoubleCheckedLocking is a performance optimization--eliminating a wait() operation on a sempahore or a monitor in most cases; the reason it doesn't always work is that it makes some gratuitious assumptions about how objects (and the registers that hold pointers to 'em) are initialized--assumptions that are violated by some aggressive compilers when coupled with concurrent programs. The "fix" is simple--just wait on the semaphore every time.

Unless the data being guarded is persistent business data, no need to involve a database. If you are guarding persistent business data; you probably have a database present already. (Or you should).

The suggestion--may I dub it TableOrientedSynchronization?--that databases ought to replace lower-level synchronization primitives is a fine example of an AbstractionInversion.

Languages, memory managers, operating systems, etc. are becoming nearly as complex as databases these days. Thus, saying that one is more or less primitive than the other can get sticky. The DB engines I used in the 80's were roughly 1/30 the size of the Java Run Time Engine.

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

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

-- Plamen Parvanov

This has the same issue.

The (instance == null) returning false doesnt guarantee that the instance is initialized completely by the Java Memory Model - http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html provides more details

This page mirrored in JavaIdioms as of April 29, 2006