CODEWITHSUNDEEP
c#multithreading
Razor
Razor

Reputation: 17498

Are the IDictionary members implemented in ConcurrentDictionary thread safe?

When calling DoStuffToDictionary(dictionaryTwo), is it safe to assume the operations within the method body including indexers, and LINQ extension methods will also be thread safe?

To phrase this question differently, could a cross thread exception or deadlock arise?

var dictionaryOne = new ConcurrentDictionary<int,int>();
var dictionaryTwo = new Dictionary<int,int>();

DoStuffToDictionary(dictionaryOne);
DoStuffToDictionary(dictionaryTwo);

void DoStuffToDictionary(IDictionary<int,int> items) {
   // Manipulate dictionary    

   if (items[0] == -1) {
     items[0] = 0; // Dumb example, but are indexers like this OK?
  }
}

Upvotes: 6

Views: 1088

Answers (3)

vgru
vgru

Reputation: 51204

There are several problems with this code:

  1. IDictionary interface can be implemented by any type of a dictionary
    Your example is certainly not thread safe, since you are working on an IDictionary<int,int> interface, which doesn't guarantee any thread safety. Even your code passes both a Dictionary and a ConcurrentDictionary to the method.

  2. Transactions need to be atomic to make them thread-safe
    Even if the dictionary implementation was guaranteed to be thread safe, your code wouldn't be because you are not locking the access to your dictionary between two calls: 

    if (items[0] == -1) {
    // <-- another thread can access items in this moment
    items[0] = 0;
}

  3. Returning a LINQ query is never thread-safe
    If you are using LINQ to return IEnumerable or IQueriable from your method, then locks have little effect, unless you use the ToList() method to evaluate the expression immediatelly and cache results. This is due to the fact that a LINQ only "prepares" the query for execution. If you are returning an IEnumerable from a method, actual dictionary will be accessed after your method ends (and therefore, outside the lock).

The biggest problem with this code lies in the fact that you are passing the IDictionary instance around, which means that other parts of your code can access it directly, and must lock on the same lock object instance very carefully. This is painful, error-prone to implement correctly, easy to break by accident, and hard to detect (race conditions may show symptoms on rare occasions).

You can do several things to improve the code:

  1. Don't pass the IDictionary around, but instead your own interface (preferred)
    Make the dictionary a private member of a class which implements some custom interface, abstract all operations, and use a lock to ensure thread safety (or use a ConcurrentDictionary under the hood). This way you are sure that all calls are being locked using the same lock instance.

  2. Don't use the interface, but rather always pass the ConcurrentDictionary
    This will be thread safe as long as you use specific atomic methods which a ConcurrentDictionary provides (GetOrAdd, AddOrUpdate, etc.). Using simple access methods like you did in your example won't be thread safe, which means you still need to be careful with it. Additional downside is that you won't be able to abstract the functionality if you ever need to (it will be impossible to wrap/proxy the dictionary, and you won't be able to use other dictionary implementations).

  3. Pass the IDictionary around, and lock on the dictionary itself (not recommended at all).
    This is an ugly hack, which is unfortunately used more often than it should be. It means you need to do this in every part of your app which accesses this dictionary, taking additional care to lock multiple operations along the way.

Upvotes: 7

Yahia
Yahia

Reputation: 70369

the lines (together)

   if (items[0] == -1) {
     items[0] = 0; // Dumb example, but are indexers like this OK?

are NOT thread-safe... with a ConcurrentDictionary this 

    ConcurrentDictionary<int, int> D = new ConcurrentDictionary<int, int>();
    D.TryUpdate(0, 0, -1); // this is threadsade regarding a ConcurrentDictionary

would be threadsafe with the result you want to achieve with the above lines.

Upvotes: 2

Jalal Said
Jalal Said

Reputation: 16162

1) For the Dictionary LINQ extension methods are not a thread safe because the class itself is not a thread safe, but for the ConcurrentDictionary it is a thread safe and so calling the LINQ extensions functions for it is thread safe .

Edit:

2) Is it safe to assume the operations within the method body including indexers I don't know what you are mean by including indexers but if you are referring to that the dictionaries is not empty. then for the Dictionary you should not consider that it contains items.. however for the ConcurrentDictionary you can use methods like: AddOrUpdate();, GetOrAdd();, TryAdd();, TryGetValue();

Upvotes: 1

Related Questions