Have I been doing locks wrong this whole time?

Question

I was reading this article about thread safety in Singletons, and it got me thinking that I don't understand the lock method.

In the second version, the author has this:

public sealed class Singleton
{
    private static Singleton instance = null;
    private static readonly object padlock = new object();

    Singleton()
    {
    }

    public static Singleton Instance
    {
        get
        {
            lock (padlock)
            {
                if (instance == null)
                {
                    instance = new Singleton();
                }
                return instance;
            }
        }
    }
}

Whereas I would've done something more like this:

public sealed class Singleton
{
    private static Singleton instance = null;

    Singleton()
    {
    }

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

Why would you use a padlock object, rather than locking the actual object you want to lock?

Answer 1

What would you expect to happen the first time you accessed the Instance property, before you've got an object to lock on?

(Hint: lock(null) goes bang...)

As a separate measure, I almost always avoid locking on "the actual object" - because typically there may well be other code which that reference is exposed to, and I don't necessarily know what that's going to lock on. Even if your version did work, what would happen if some external code wrote:

// Make sure only one thread is ever in this code...
lock (Singleton.Instance)
{
    // Do stuff
}

Now no-one else can even get the instance while that code is executing, because they'll end up blocked in the getter. The lock in the getter isn't meant to be guarding against that - it's only meant to be guarding against multiple access within the getter.

The more tightly you can keep control of your locks, the easier it is to reason about them and to avoid deadlocks.

I very occasionally take a lock on a "normal" object if:

• I'm not exposing that reference outside that class
• I have confidence that the type itself (which will always have a reference to this, of course) won't lock on itself.

(All of this is a reason to avoid locking on this, too, of course...)

Fundamentally, I believe the idea of allowing you to lock on any object was a bad idea in Java, and it was a bad move to copy it in .NET :(

Answer 2

Locking on this or any other non-private object is dangerous because it can lead to deadlocks if someone else also tries to use that object for synchronization.

It's not terribly likely, which is why people can be in the habit of doing it for years without ever getting bitten. But it is still possible, and the cost of a private instance of object probably isn't great enough to justify running the risk.