Is this using of dictionary thread-safe or not?

Go To StackoverFlow.com

3

Here is two my variants of method, which returns a string, associated with enum value (kept in dictionary). First variant is slower, but thread-safe, second is faster, but i don't know, whether it is thread-safe or not. First:

string GetStringForEnum (SomeEnum e)
{
   string str = null;
   lock (someDictionary) //someDictionary is not used anywhere else (only in this method)
   { if (!someDictionary (e, out str)) { someDictionary.Add (e, "somehowCreatedString"); }
   return str;
}

Second variant:

string GetStringForEnum (SomeEnum e)
{
   string str = null;
   if (!someDictionary (e, out str))
   {
     lock (someDictionary) //someDictionary is not used anywhere else (only in this method)
     { if (!someDictionary (e, out str)) { someDictionary.Add (e, "somehowCreatedString"); }
   }
   return str;
}

Second variant is not used "lock" each time, but is it thread-safe or not?

2012-04-04 08:05
by Vasya
Is this suppose to be real code? I can't place !someDictionary (e, out str)Henk Holterman 2012-04-04 08:09


4

There are 2 problems here:

  • lock (someDictionary) - this is dis-advised, even if the dictionary is not used elsewhere. It is a theoretical argument but the (future) code of the Dictionary class could lock on itself.

  • if (!someDictionary (e, out str)) without a lock. I assume this is a call to TryGetValue(). This simply is not thread-safe, your Read could be interrupted by a Write in another thread. This could end in all sorts of errors (index out of range, null reference). The errors will be very rare (= hard to reproduce).

2012-04-04 08:13
by Henk Holterman
"but the (future) code of the Dictionary class could lock on itself.". If dictionary was to do that, it would break a strong guideline of the Framework Design Guidelines and this would also be a breaking change. It is therefore safe to assume that dictionary will never lock on itself - Steven 2012-04-04 08:23
you should also be aware that you'll need a double check when using locks. - Boas Enkler 2012-04-04 08:33
@Steven - you're right, like is said 'theoretical'. The best practice is to use an instance that you control and hide as much as possible - Henk Holterman 2012-04-04 08:50
@HenkHolterman: I must say I'm very pragmatic about this when writing LOB applications and simply lock on the private collection (when possible). On the other hand I'm very strict, when it comes to writing code for reusable libraries. It's all about context ;- - Steven 2012-04-04 09:23
"The errors will be very rare (= hard to reproduce)." - Yu - ProVega 2013-10-23 18:07


3

The documentation on Dictionary has a section on thread safety:

A Dictionary(Of TKey, TValue) can support multiple readers concurrently, as long as the collection is not modified. Even so, enumerating through a collection is intrinsically not a thread-safe procedure. In the rare case where an enumeration contends with write accesses, the collection must be locked during the entire enumeration. To allow the collection to be accessed by multiple threads for reading and writing, you must implement your own synchronization.

For a thread-safe alternative, see ConcurrentDictionary(Of TKey, TValue).

So:

  • TryGetValue is safe to use from multiple threads because it is read-only. However, it is not safe when other code is writing the dictionary at the same time (which your code is doing).
  • Adding a value is never safe unless you lock the dictionary.
  • Using a ConcurrentDictionary is an easy solution to get going, but it may be no faster than your first version (I assume it's going to lock on every operation).

And a side note: Using a field that is not private (here the field is someDictionary and we don't know if it's private or not) as a lock target is not recommended because theoretically external code could also decide to lock it without your knowledge (practically this is not going to happen, but why not be theoretically correct as well?).

2012-04-04 08:08
by Jon
Small correction. You say "TryGetValue is safe to use from multiple threads because it is read-only.", but this only holds "as long as the collection is not modified". The examples given by the OP clearly show that he actually IS updating the dictionary, which make it unsafe to use TryGetValue outside a lock.unsafe. For the rest of your answer: +1 - Steven 2012-04-04 08:20
@Steven: I should probably have made that explicit. Edited the answer accordingly, thanks for the feedback - Jon 2012-04-04 08:28


2

If you use .NET 4 you can use ConcurrentDictionary for thread safety.

2012-04-04 08:08
by Marcus
Yeah, I would strongly recommend using the Collections from the System.Collections.Concurrent namespace, instead of using a lock.

If not possible then use ReaderWriterLockSlim - Boas Enkler 2012-04-04 08:32

Ads