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…

kick it on DotNetKicks.com

Similar Posts:

    None Found


2 Comments to “What’s wrong with this code? #1 – Discussion”

  1. Chan | March 28th, 2009 at 2:30 am

    Hi,
    I am using static constructor as below, it hit error if Eval() is called by more than one person at the same time. It works time if I added lock(). However, as you mentioned static constructor should be thread safe. Shall you please explain about this ?

    Thank you

    public class Evaluator
        {
            private static object _evaluator = null;
            private static Type _evaluatorType = null;
            private static readonly string _jscriptSource = @"
                                                            package Evaluator
                                                            {
                                                                class Evaluator
                                                                {
                                                                    public function Eval(expr : String) : String
                                                                    {
                                                                        return eval(expr,""unsafe"");
                                                                    }
                                                                }
                                                            }";
    
            static Evaluator()
            {
                CodeDomProvider provider = CodeDomProvider.CreateProvider("JScript");
    
                CompilerParameters parameters = new CompilerParameters();
                parameters.GenerateInMemory = true;
    
                CompilerResults results = provider.CompileAssemblyFromSource(parameters, _jscriptSource);
    
                Assembly assembly = results.CompiledAssembly;
                _evaluatorType = assembly.GetType("Evaluator.Evaluator");
    
                _evaluator = Activator.CreateInstance(_evaluatorType);
            }
    
            public static object EvalToObject(string expression)
            {
                object result;
    
                try
                {
                    result = _evaluatorType.InvokeMember(
                        "Eval",
                        BindingFlags.InvokeMethod,
                        null,
                        _evaluator,
                        new object[] { expression }
                        );
                }
                catch (Exception ex)
                {
                    throw new Exception(string.Format("Evaluator: Failed to evaluate expression {0} - {1}", expression, ex.Message));
                }
    
                return result;
            }
    
            public static string Eval(string expression)
            {
                object o = EvalToObject(expression);
                return o.ToString();
            }
        }
    
  2. ekampf | March 28th, 2009 at 6:19 pm

    Hi Chan,
    As specified in MSDN C# spec (http://msdn.microsoft.com/en-us/library/aa645612.aspx) static constructors run at most once per app domain. Its even more specific in “Managed Threading Best Practices” (http://msdn.microsoft.com/en-us/library/1c9txz50(VS.80).aspx):

    “A class is not initialized until its class constructor (static constructor in C#, Shared Sub New in Visual Basic) has finished running. To prevent the execution of code on a type that is not initialized, the common language runtime blocks all calls from other threads to static members of the class (Shared members in Visual Basic) until the class constructor has finished running.

    For example, if a class constructor starts a new thread, and the thread procedure calls a static member of the class, the new thread blocks until the class constructor completes.

    This applies to any type that can have a static constructor.”

    So, static constructors are definitely thread safe and you’ll probably hitting some other error… What kind of exception are you getting?