Code Review: Double Checked Locking Code






A friend of mine recently sent me some code to review:


 


Hi Peter,


Do you have any suggestions on how to clean up this code?


In particular the creation of a particular singleton type doesn’t look very pretty in this model:


 


private DoubleCheckedLock<SingletonClass> myInstance = new DoubleCheckedLock<SingletonClass>();


public SingletonClass Current {


    get {


        return this.myInstance.GetSingleton(


            delegate() {


                return this.CreateSingleton(OrderContext, “third”);


            }


        );


    }


}


 


I’m sharing my comments on the code, as it demonstrates some important design principles. Here’s the original code:


 


public class DoubleCheckedLock<T> where T : class


{


    private volatile T myInstance;


    private static readonly object lockObject = new object();


 


    public T GetSingleton(CreateObject c) {


        if (this.myInstance == null) {


            lock (lockObject) {


                if (this.myInstance == null) {


                    this.myInstance = c();


                }


            }


        }


        Debug.Assert(this.myInstance != null);


        return this.myInstance;


    }


 


    public delegate T CreateObject();


 


}


 


public abstract class DoubleCheckedLock2<T> where T : class


{


    protected abstract T CreateObject();


    private DoubleCheckedLock<T> myInstance = new DoubleCheckedLock<T>();


 


    public T Instance {


        get {


            return this.myInstance.GetSingleton(this.CreateObject);


        }


    }


}


 


And here are my comments:


 



  • The name DoubleCheckedLock describes the implementation. Names should reflect how the client will use a symbol, not how something is implemented. For clients, the class implements a lazily created, thread safe singleton object. Aka, a Singleton object that does the right thing. Naming the class ‘Singleton’ will make client code read much better.

 



  • I’m not a fan of public nested types, so I’d recommend moving the CreateObject delegate to a top level type. That’s a style choice, but I’ve found it to wear well in practice.

 



  • Names of types, including delegate types, should be nouns, not verbs or verb phrases. CreateObject should be renamed Creator.

 



  • Passing in the creator delegate is necessary for this pattern so don’t sweat it. However, passing in the delegate to the ‘GetSingleton‘ method allows for some surprising behavior. It allows different calls to ‘GetSingleton‘ to supply different values which will surely cause confusion. This confusion can be designed out by passing in the creator delegate as an argument to the Singleton constructor.

 



  • Once the GetSingleton parameter is removed, it can become a property named ‘Value’. Again, this makes your client code read better.

 



  • Add the ‘sealed’ and ‘readonly‘ modifiers unless you are specifically expecting to use a class as a base or modify a field after construction. The additional modifiers self document the code, and make you think through your design more fully.

 



  • The DoubleCheckedLock2 abstract base class is a classic example of inheriting for containment – representing a ‘has a’ relationship between the derived class and the DoubleCheckedLock2 base class. Inheritance should always represent an ‘is a’ relationship, never a ‘has a’ relationship. You will save yourself many headaches by avoiding base classes which force you to use your one precious base class to represent a ‘has a’ relationship. Get rid of the DoubleCheckedLock2 class. The fact that it has such a crummy name, and that there is no good name to give this thing is a clear indication that something is wrong with it.

 


Now the client code looks like this:


 


sealed class Client {


    private readonly Singleton<ExpensiveResource> expensiveValue;


    Client() {


        this.expensiveValue =


                new Singleton<ExpensiveResource>(


                    delegate()


                    {


                        return new ExpensiveResource();


                    }


                );


    }


 


    public ExpensiveResource ExpensiveValue {


        get { return this.expensiveValue.Value; }


    }


}


 


Which is pretty damn sexy, if I don’t say so myself.


 


Here’s the implementation:


 


public delegate T Creator<T>();


 


public class Singleton<T> where T : class {


    private static readonly object lockObject = new object();


    private volatile T myInstance;


    private readonly Creator<T> creator;


 


    public Singleton(Creator<T> creator) {


        Debug.Assert(creator != null);


        this.creator = creator;


    }


 


    public T Value {


        get {


            if (this.myInstance == null) {


                lock (lockObject) {


                    if (this.myInstance == null) {


                        this.myInstance = this.creator();


                        this.creator = null;    // allow creator object to be GC-ed as soon as possible


                    }


                }


            }


            Debug.Assert(this.myInstance != null);


            return this.myInstance;


        }


    }


}


 


Happy coding,


Peter


Comments (4)

  1. if creator is readonly, shouldnt it be modifiable only inside your constructor? Your set to null should fail on compile.

    Furthermore, you could very well get rid of the volatile and do a memorybarrier on assignment, which is very much semantically correct.

    One question though, if you want to encapsulate the creation of Singleton<T>, and i’ll assume it’s of type T, is it really necessary to roundtrip through the use of a creator? Wouldn’t a good old factory be appropriate enough?

    Final little question, why the need to put the SyncRoot (uh lockObject) as static? Seems if a few of your objects had to initialize at the same time you could end up in a contention situation on the lock rather than on the resources themselves. Especially as the cost of initializing T grows?

    SerialSeb

  2. One additional problem in the original code is the delegate allocation on every access to the singleton. Moving this to a constructor corrects this.

    I think SingltonCreator is a better name for the delegate, however. Creator is too general. I would only leave it as Creator if it is a nested type. Speaking of which, I don’t think nested delegate types are as bad as they were (at least from C#) since the addition of anonymous delegates and the implicit new (though the implicit new hides the allocation above).

    But, I think the name Singleton misses the point. The point of the class is not to create a singleton but to create lazy constructed object (or, more correctly, a lazily evaluated expression). A singleton implies one instance of the class exists. This is not what this does. This is just version of a smart pointer that lazily evaluates the assignment expression. Lazy<T> would be better in my opinion.

    To answer some of Sebastien’s comments, the readonly is a bug in the code, it fails to compile has he surmised.

    I am not convinced a memory barrier is sufficient here since I believe it is important to only call the delegate once. Only a lock prevents that.

    As for using lockObject, why not just lock on the delegate? That seems the most straight forward approach instead of maintaining a private lock object. It also what you are really trying to do is guard the delegate invocation and not the write since the write guard can be avoided with an Interlocked.CompareExchange<T>() if you were willing to execute the delegate on each thread and throw away duplicate work from thread that lose the race.

    ChuckJ

  3. Agreed on the lock.

    On the memory barrier, it’s sufficient to prevent the use of volatile, not the double lock.

  4. This week the focus is on the launch of Visual Studio 2005 Service Pack 1 (VS 2005 SP1) and the impending