Volatile keyword and locking for instance properties

Question

I have some code for the instance property of a controller class that looks like this:

public class Controller
{
    private static volatile Controller _instance;
    private static object syncRoot = new Object();

    private Controller() { }

    public static Controller Instance
    {
        get
        {
            if (_instance == null)
            {
                lock (syncRoot)
                {
                    if (_instance == null)
                        _instance = new Controller();
                }
            }

            return _instance;
        }
    }

    public void Start()
    {

    }
}

After reading through the msdn docs on the volatile keyword, I'm not sure if the second null check is redundant or not and whether the better way to write the getter would be something like this:

get
{            
    lock (syncRoot)
    {
        if (_instance == null)
            _instance = new Controller();
    }            

    return _instance;
}

Which of the two implementations are better for multi-thread performance and DRY'ness (redundancy removal)?

Answer 1

This is called the "double checked locking" pattern. It is an attempt at a low-lock optimization and is therefore extremely dangerous.

The pattern is not guaranteed to work correctly on CLR v1.0. Whether it is so guaranteed on later versions is a matter of some debate; some articles say yes, some say no. It is very confusing.

I would avoid it entirely unless you had a very good reason to suppose that the solution with locking was insufficient to meet your needs. I would use higher-level primitives, like Lazy<T>, written by experts like Joe Duffy. They're more likely to be correct.

This question is a duplicate of

The need for volatile modifier in double checked locking in .NET

See the detailed answers there for more information. In particular, if you ever intend to write any low-lock code you absolutely have to read Vance's article:

http://msdn.microsoft.com/en-us/magazine/cc163715.aspx

and Joe's article:

http://www.bluebytesoftware.com/blog/PermaLink,guid,543d89ad-8d57-4a51-b7c9-a821e3992bf6.aspx

Note that Vance's article makes the claim that double-checked locking is guaranteed to work in CLR v2. It is not clear to me that the guarantees discussed in the article were actually implemented in CLR v2 or not; I have never gotten a straight answer out of anyone and have heard both that they were and were not implemented as specified. Again, you're on the trapeze without a net when you do this low-lock stuff yourself; avoid it if you possibly can.

Answer 2

A much better option would be to use Lazy<T>:

private static readonly Lazy<Controller> _instance = new Lazy<Controller>
                                               (() => new Controller());
private Controller() { }

public static Controller Instance
{
    get
    {
        return _instance.Value;
    }
}

If, however, you're stuck with a version prior to .NET 4, I'd recommend reading Jon Skeet's article on Singletons in C#. It discusses the advantages and disadvantages to the above techiques, as well as providing a better implementation of a lazy instantiated singleton for .NET 3.5 and earlier.

Answer 3

the classic way to do this is double-checked locking: You check once before acquiring the lock to reduce overhead (acquiring the lock is relatively expensive) - then check again after you got the lock to be sure it wasn't set already.

Edit: As was pointed out Lazy<T> is a much better option here, I'm leaving this answer here for completeness.

if (_instance == null)
{
    lock (syncRoot)
    {
        if (_instance == null)
            _instance = new Controller();
    }            
}

return _instance;

Answer 4

you want to check before locking the object so that no unnecessary locking occurs you also want to check after locking to avoid multiple instantiations of the object

the first is not needed but highly advisable for performance sake