Choosing What To Lock On

I said earlier that I'd explain why I created a new variable to lock on. Many books and articles recommend locking on this for instance methods and typeof(MyTypeName) (the Type object for whatever type you're writing code in) for static methods. I believe this is a bad idea, because it means you have less control over your locks. Other code may well end up locking on the same object as you do within your code, which makes it far harder to ensure that you only obtain locks in an appropriate order.

An alternative to locking on this is to lock on the reference of the object you're about to access - and indeed ICollection provides a SyncRoot property for exactly this purpose, providing a "shared" reference for different callers to lock on. This is a valid design in some situations, but should generally be avoided - if you keep your locks as private as possible, you should be able to write thread-safe objects when you wish to. The difficulty I believe ICollection is trying to solve is providing a single class which is fast in a single-threaded situation due to not locking internally but which allows easy thread-safe access when in a multi-threading situation. It also enables a common reference to be used for locking across several method calls, such as enumerating the whole collection. (Obviously ICollection itself doesn't provide any code for any of this as it's just an interface, but it encourages a consistent design for implementing classes to follow.)

In my experience, this level of complexity is unusual when developing applications - usually the ability to make each method thread-safe in itself is all that's required, and that can be achieved through entirely "private" locks, which no other object knows about. Even if you never expose your fields directly, just accessing them for callers, you don't usually know whether methods within those objects have decided to lock on this, so for absolute control you should create a new variable, and immediately instantiate a new object. This is the object you end up locking on. For "static locks" you should declare a private static variable (and immediately instantiate a new object) for the same reason - other classes can get a Type reference to your type too! While it is unlikely that they would lock on your type, it's not impossible - a private static variable keeps the lock invisible to other classes. In both cases, make the variable read-only to stop yourself from accidentally changing the value.

One other practice occasionally seen is that of locking on string literals, for example declaring string someLock = "Lock guarding some data"; and then locking on someLock. This has the same problem as the other locking strategies, due to string interning - if two classes both use the same string literal, they'll end up using references to the same string, so they'll be sharing a lock without realising it. If you really want to make a lock with a description, you can always create your own Lock class. (This would also have the benefit of making it obvious to the rest of your code what the appropriate variable is used for - Lock is more descriptive than object in itself.)

In any one class you may have several locks, dealing with different bits of data which need to remain consistent. The more locks you have, the more finely grained your locking is - but the more complicated it gets making sure that you always take them out in a consistent order. (You also end up using more memory per instance, of course, but that's usually not an issue.) Of course, one of the benefits about having variables just for locking is that they can get their own XML documentation, making it easier to understand what pieces of state each lock is used to cover.

One final note on the issue: not only do many books and articles recommend locking on this: the C# compiler does it for you automatically if you declare an event without specifying the add/remove code. My recommendation is to explicitly write the add/remove code, following a pattern something like this:

/// <summary>
/// Delegate backing the SomeEvent event.
/// </summary>
SomeEventHandler someEvent;

/// <summary>
/// Lock for SomeEvent delegate access.
/// </summary>
readonly object someEventLock = new object();

/// <summary>
/// Description for the event
/// </summary>
public event SomeEventHandler SomeEvent
{
    add
    {
                  lock (someEventLock)
        {
            someEvent += value;
        }
    }
    remove
    {
                  lock (someEventLock)
        {
            someEvent -= value;
        }
    }
}

/// <summary>
/// Raises the SomeEvent event
/// </summary>
protected virtual OnSomeEvent(EventArgs e)
{
    SomeEventHandler handler;
                  lock (someEventLock)
    {
        handler = someEvent;
    }
                  if (handler != null)
    {
        handler (this, e);
    }
}

(This assumes that the SomeEventHandler delegate has been declared elsewhere.) Most of the time you can use a single lock for all your events, in my experience. Note the code for OnSomeEvent. It's important that you don't write it as:

// Bad code! Do not use!
protected virtual OnSomeEvent(EventArgs e)
{
                  lock (someEventLock)
    {
                  if (someEvent != null)
        {
            someEvent(this, e);
        }
    }
}

or:

// Bad code! Do not use!
protected virtual OnSomeEvent(EventArgs e)
{
                  if (someEvent != null)
    {
        someEvent (this, e);
    }
}

The first ends up firing the event while still holding the lock, which is a bad idea - you don't know what code is going to be run at this stage, and it could well end up wanting to access the event from another thread, leading to deadlock.

The second doesn't lock at all, making it quite possible that the event delegate will change between the test for nullity and the invocation. That wouldn't be a problem most of the time, but if the delegate variable's value becomes null, then trying to invoke the delegate will lead to an exception being thrown. (If we didn't care whether the delegate variable's value was null or not, we wouldn't test it in the first place.)

Back to the main C# page.