An extension method to safely retrieve or add a dictionary value

Question

The extension method:

/// <summary>
/// Thread safe. May lock on the input dictionary.
/// </summary>
public static U GetOrAdd<T, U>(this Dictionary<T, U> dict, T key, Func<U> create) {
    U val;
    if (!dict.TryGetValue(key, out val)) {
        lock (dict) {
            if (!dict.ContainsKey(key)) {
                val = create();
                dict[key] = val;
            } else {
                val = dict[key];
            }
        }
    }
    return val;
}

Sample usage:

var idVals = new Dictionary<int, List<int>>();
List<int> vals = idVals.GetOrAdd(id, () => new List<int>());
vals.Add(3);

Explanation:

• If the key exists, we quickly retrieve the value using the TryGetValue() method
• If the key doesn't exist we use the provided create delegate to make a new one
• We are protected against the case of two threads simultaneously looking for a key that doesn't exist by locking on the input dictionary and afterward re-checking to see if the key still doesn't exist

Questions:

• Are there scenarios that will cause this fail?
• I found this extension method to be useful, but is clear enough to be properly understood by others?
• Are there easier ways to achieve the thread-safety I'm looking for?
• I know it's considered dangerous and bad practice to lock on an input parameter, but can I rely on the fact that locking on a dictionary to safely add/remove elements is a common convention?

Answer 1

The biggest problem with your code is that it's not thread-safe. You can't just access dictionary without a lock for reading and only lock it for writing. The problem is that while your code is reading, another thread could be writing to the dictionary and that means the reading could return bogus data/throw exception/whatever.

If you're trying to mimic double-checked locking, then that only works because the read there is atomic. But TryGetValue is not atomic, so this approach doesn't work here.

You could make the code thread-safe by using ReaderWriterLockSlim. You would enter the lock in the upgradeable mode first, then call TryGetValue and if you find out you need to write, upgrade it to write mode. Though this requires the user to use the same ReaderWriterLockSlim too and it only makes sense if the lock is often used in the read mode (so that the readers will work concurrently with your method; two threads in upgradeable mode can't work concurrently).

But this is a pretty complicated solution with uncertain benefits. I think a better option is just to lock the whole method. And in that case, making the method thread-safe makes even less sense (see also below).

---

can I rely on the fact that locking on a dictionary to safely add/remove elements is a common convention

I don't think you should.

It seems your code is supposed to be used like this:

lock (dictionary)
{
    dictionary[42] = "forty two";
}

var result = dictionary.GetOrAdd(42, () => "42");

Code like this would make me very nervous, because it looks like the call to GetOrAdd() is missing a lock.

Thread-safe code that doesn't need a lock (like ConcurrentDictionary) is great. Code that requires a lock always is still good: that's a simple rule, it's easy to remember, follow and check. Code that requires a lock only sometimes is bad, because it means the developer has to remember which methods are thread-safe and which aren't. And that's likely going to lead to bugs ("dictionary.GetOrAdd() doesn't need a lock, so dictionary.Add() doesn't either, right?").

---

Another issue is that sometimes you don't want to lock on the same object. It's not unusual to have two fields that need to be kept synchronized, so both share the same lock; your code doesn't allow that. Another possibility is that the user code is using another locking mechanism (ReaderWriterLockSlim, SpinLock, …) and your code won't work with that either.

---

The same method on ConcurrentDictionary uses Func<TKey, TValue>, not just Func<TValue>. This is done to avoid closure for code like:

dictionary.GetOrAdd(key, () => someFunction(key));

Instead, it's written as:

dictionary.GetOrAdd(key, k => someFunction(k));

which avoids allocating a temporary object for the closure. If performance/GC pressure is not a concern for you, you can ignore this.

Answer 2

Are there easier ways to achieve the thread-safety I'm looking for?

Yes, use the ConcurrentDictionary Class which is avaible since NET 4.0