What's wrong with this code? #1 - Discussion

The Singleton implementation in the snippet I posted works fine as a lazy, thread-safe Singleton as it ensures only one thread can create the instance. However, there’s a big performance hit caused by the fact that we acquire a lock each time the Singleton’s instance is requested. Yoav, suggested to fix this performance problem by checking for null twice – outside the lock and inside the lock:

public static Singleton Instance
{
    get
    {
        if (Singleton._insatnce == null)
        {
            lock (_syncRoot)
            {
                if (Singleton._insatnce == null)
                {
                    Singleton._insatnce = new Singleton();
                }
            }
        } return Singleton._insatnce;
    }
}

This code, known as “Double-Check Locking” doesn’t work in the presence of either an optimizing compiler or shared memory multiprocessor. The main reason for this (as explained by Brad) is that the CLR’s memory model allows non-volatile read\writes to be reordered as long as that change cannot be noticed from the point of view of a single thread. This means that the compiler can reorder write to _instance during initialization and its construction write, causing another thread to see _instance as set even though it wasn’t initialized yet (appears on stress testing). To prevent such optimizations we can declare _instance to be volatile or by explicitly specifying a memory barrier before accessing the data member using System.Threading.Thread.MemoryBarrier(). Using Thread.MemoryBarrier() is more efficient than volatile as it allows compiler optimization when barrier is not required:

public static Singleton Instance
{
    get
    {
        if (Singleton._insatnce == null)
        {
            lock (_syncRoot)
            {
                if (Singleton._insatnce == null)
                {
                    Singleton newInstance = new Singleton(); // Ensure all writes used to construct new value have been flushed.
          System.Threading.Thread.MemoryBarrier();
          Singleton._insatnce = newInstance; // publish the new value
                }
            }
        } return Singleton._insatnce;
    }
}

Another way of doing the same thing is using System.Threading.Interlocked.CompareExchange() which also enforces a memory barrier.

Omer, suggested creating the instance of Singleton in the static constructor (which is guaranteed to be thread-safe) rather than in the Instance accessor. While in this implementation you lose some of the flexibility of controlling exactly at which point in time the Singleton instance is created, this kind of control is not really required in most cases and giving up on it allows making the code shorter and less confusing…