CODEWITHSUNDEEP
c#.netthread-safety
Tadeusz
Tadeusz

Reputation: 6873

Is this using of dictionary thread-safe or not?

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?

Upvotes: 3

Views: 2918

Answers (3)

Jon
Jon

Reputation: 437326

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?).

Upvotes: 3

Henk Holterman
Henk Holterman

Reputation: 273169

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).

Upvotes: 4

Marcus
Marcus

Reputation: 2480

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

Upvotes: 2

Related Questions