Any downsides to locking a collection vs. a syncRoot?

Question

I'm wondering if there are any downsides to locking over a collection such as a List<T>, HashSet<T>, or a Dictionary<TKey, TValue> rather than a simple object.

Note: in the following examples, that is the only place where the locks occur, it's not being locked from multiple places, but the static method may be called from multiple threads. Also, the _dict is never accessed outside of the GetSomething method.

My current code looks like this:

private static readonly Dictionary<string, string> _dict = new Dictionary<string, string>();
public static string GetSomething(string key)
{
    string result;
    if (!_dict.TryGetValue(key, out result))
    {
        lock (_dict)
        {
            if (!_dict.TryGetValue(key, out result))
            {
                _dict[key] = result = CalculateSomethingExpensive(key);
            }
        }
    }
    return result;
}

Another developer is telling me that locking on a collection will cause issues, but I'm skeptical. Would my code be more efficient if I do it this way?

private static readonly Dictionary<string, string> _dict = new Dictionary<string, string>();
private static readonly object _syncRoot = new object();
public static string GetSomething(string key)
{
    string result;
    if (!_dict.TryGetValue(key, out result))
    {
        lock (_syncRoot)
        {
            if (!_dict.TryGetValue(key, out result))
            {
                _dict[key] = result = CalculateSomethingExpensive(key);
            }
        }
    }
    return result;
}

Answer 1

No. As long as the variable is not accessible from anywhere else, and you can guarantee that the lock is only used here, there is no downside. In fact, the documentation for Monitor.Enter (which is what a lock in C# uses) does exactly this.

However, as a general rule, I still recommend using a private object for locking. This is safer in general, and will protect you if you ever expose this object to any other code, as you will not open up the possibility of your object being locked on from other code.

Answer 2

If you expose your collections to the outside world, then, yes this can be a problem. The usual recommendation is to lock on something that you exclusively own and that can never be locked unexpectedly by code that is outside your influence. That's why generally it's probably better to lock on something that you'd never even consider exposing (i.e. a specific lock object created for that purpose). That way, when your memory fails you, you'll never probably not get unexpected results.

To answer your question more directly: Adding another object into the mix is never going to be more efficient, but placing what is generally regarded as good coding practice before some perceived, but unmeasured efficiency might be an optmisation occurring prematurely. I favour best practice until it's demonstrably causing a bottleneck.

Answer 3

To directly answer your question: no

It makes no difference whatever object you're locking to. .NET cares only about it's reference, that works exactly like a pointer. Think about locking in .NET as a big synchronized hash table where the key is the object reference and the value is a boolean saying you can enter the monitor or not. If two threads lock onto different objects (a != b) they can enter the lock's monitor concurrently, even if a.Equals(b) (it's very important!!!). But if they lock on a and b, and (a==b) only one of them at a time will be in the monitor.

As soon as dict is not accessed outside your scope you have no performance impact. If dict is visible elsewhere, other user code may get a lock on it even if not necessarily required (think your deskmate is a dumb and locks to the first random object he finds in the code).

Hope to have been of help.

Answer 4

I would recommend using the ICollection.SyncRoot object for locking rather than your own object:

    private static readonly Dictionary<String, String> _dict = new Dictionary<String, String>();
    private static readonly Object _syncRoot = ((ICollection)_dict).SyncRoot;

Answer 5

In this case, I would lock on the collection; the purpose for the lock relates directly to the collection and not to any other object, so there is a degree of self-annotation in using it as the lock object.

There are changes I would make though.

There's nothing I can find in the documentation to say that TryGetValue is threadsafe and won't throw an exception (or worse) if you call it while the dictionary is in an invalid state because it is half-way through adding a new value. Because it's not atomic, the double-read pattern you use here (to avoid the time spent obtaining a lock) is not safe. That will have to be changed to:

private static readonly Dictionary<string, string> _dict = new Dictionary<string, string>();
public static string GetSomething(string key)
{
    string result;
    lock (_dict)
    {
        if (!_dict.TryGetValue(key, out result))
        {
            _dict[key] = result = CalculateSomethingExpensive(key);
        }
    }
    return result;
}

If it is likely to involve more successful reads than unsuccessful (that hence require writes), use of a ReaderWriterLockSlim would give better concurrency on those reads.

Edit: I just noticed that your question was not about preference generally, but about efficiency. Really, the efficiency difference of using 4 bytes more memory in the entire system (since it's static) is absolutely zero. The decision isn't about efficiency at all, but since both are of equal technical merit (in this case) is about whether you find locking on the collection or on a separate object is better at expressing your intent to another developer (including you in the future).