6
\$\begingroup\$

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?
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm wondering, in what kind of cases do you use this method? I mean, Get and Add serve two very different purposes \$\endgroup\$ – IEatBagels Oct 8 '14 at 14:41
  • 1
    \$\begingroup\$ You need a ReaderWriterLock to prevent reads during writes. \$\endgroup\$ – SLaks Oct 8 '14 at 14:42
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The double-locking is safe since .NET 2.0 as you can read in your own link (was not safe in .NET 1.1, but that is history) \$\endgroup\$ – user52292 Oct 9 '14 at 19:28
  • \$\begingroup\$ @firda It is safe when you used for singleton (with mySingleton == null outside the lock). But it's not safe here (with TryGetValue outside the lock). \$\endgroup\$ – svick Oct 9 '14 at 19:31
  • \$\begingroup\$ You mean that double-locking is fine for GetOrAdd but not for any other non-locked access? That would be (my) missunderstanding (of your description). The double-locking itself is safe. \$\endgroup\$ – user52292 Oct 9 '14 at 19:32
  • \$\begingroup\$ @firda I mean it's not fine the way Ami used it. You can't call TryGetValue on one thread while also modifying the dictionary on another thread, which is what this code does when used from multiple threads. \$\endgroup\$ – svick Oct 9 '14 at 19:46
  • \$\begingroup\$ Got it, my fault, was not reading carefully enough ;) The main problem here is that it should not be extension method, because it is not safe to be used as such - the whole solution needs special class, not extension method. Your description is little too long so I missed the meaning at some point. Still +1 because you have described the problem (instead of simple "Hey, you should use ConcurrentDictionary"). \$\endgroup\$ – user52292 Oct 9 '14 at 19:49
8
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ I should have thought of that. \$\endgroup\$ – Ami Oct 8 '14 at 14:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.