I want to make my code multithreadable, therefor i need to change a Dictionary into a ConcurrentDictionary. I read about the ConcurrentDictionary, checked some example, but still I need a hand on this:

I'm not so sure this is it, in particular i am quite sure that the IsEmpty check is not threadsafe since the _tasks may have been initialized between the IsEmpty check and the && ... part or the return _tasks part. Do I have to lock this check manually? Do i need a double lock (null check > lock > null check) ?

2 Answers
2

Your concern is justified. The Tasks property getter is not thread-safe. There are a few issues here.

First, like you side, there is a race between a call to IsEmpty from one thread and the removal of item from another thread. The getter could return an empty dictionary.

Second, there is a race between the read of _lastTaskListRefreshDateTime in the if check and the assignment at the end of the getter. Even if these operations are atomic (which they cannot be at least on 32-bit platforms since DateTime is 64-bits) there is still a subtle memory barrier issue since no synchronization mechanisms like volatile are apparent in the code.

Third, similar to my explanation above, there is another memory barrier problem with _tasks reference. One thread could call the setter while another is calling the getter. Since no memory barrier is present the CLR or hardware are free to optimize the reads and writes in such a manner that the changes made in the setter are not visible to the getter. This issue might not necessarily cause any problems, but I bet it is behavior that was not anticipated. With no other context for analysis I cannot say either way.

Understood. So best would be to lock the whole Task getter. Locking the foreach loop only (as proposed by Gabe) wouldn't solve the issue with _lastTaskListRefreshDateTime... right?
–
StefanMay 19 '11 at 10:32

my solution is derived from Gabe's proposal to lock the filling-loop and the brian's insights, especially the race on _lastTaskListRefreshDateTime, so what i came up with is a locking of the whole getter.
–
StefanMay 24 '11 at 6:27

The ConcurrentDictionary only guarantees that reads and writes into the dictionary will not walk all over each other, something the Dictionary class does not do. The thread safety in the ConcurrentDictionary does not make your code thread safe, it only ensures its code is thread safe. Since this is the case you will need a lock in your getter.