Refactoring Dictionary to ConcurrentDictionary

Question

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:

Here is the original code (for single thread)

private IDictionary<string, IDictionary<string, Task>> _tasks;
public override IDictionary<string, IDictionary<string, Task>> Tasks
    {
        get
        {
            // return dictionary from cache unless too old
            // concurrency!! (null check)
            if (_tasks != null && (DateTime.Now - _lastTaskListRefreshDateTime < TimeSpan.FromSeconds(30)))
            {
                return _tasks;
            }

            // reload dictionary from database
            _tasks = new Dictionary<string, IDictionary<string, Task>>();

            // find returns an IEnumerable<Task>
            var tasks = Find<Task>(null, DependencyNode.TaskForCrawler).Cast<Task>();

            // build hierarchical dictionary from flat IEnumerable
            // concurrency!!
            foreach (var t in tasks)
            {

                if (_tasks.ContainsKey(t.Area.Key))
                {
                    if (_tasks[t.Area.Key] == null)
                    {
                        _tasks[t.Area.Key] = new Dictionary<string, Task>();
                    }

                    if (!_tasks[t.Area.Key].ContainsKey(t.Key))
                    {
                        _tasks[t.Area.Key].Add(t.Key, t);
                    }
                }
                else
                {
                    _tasks.Add(t.Area.Key, new Dictionary<string, Task> { { t.Key, t } });
                }
            }

            _lastTaskListRefreshDateTime = DateTime.Now;
            return _tasks;
        }

        set
        {
            _tasks = value;
        }
    }

Here is what i came up with:

private ConcurrentDictionary<string, ConcurrentDictionary<string, Task>> _tasks = new ConcurrentDictionary<string, ConcurrentDictionary<string, Task>>();
public override ConcurrentDictionary<string, ConcurrentDictionary<string, Task>> Tasks
{
        get
        {
            // use cache
            // concurrency?? (null check)
            if (!_tasks.IsEmpty && (DateTime.Now - _lastTaskListRefreshDateTime < TimeSpan.FromSeconds(30)))
            {
                return _tasks;
            }

            // reload
            var tasks = Find<Task>(null, DependencyNode.TaskForCrawler).Cast<Task>();

            foreach (var task in tasks)
            {
                var t = task; // inner scope for clousure
                var taskKey = t.Key;
                var areaKey = t.Area.Key;

                var newDict = new ConcurrentDictionary<string, Task>();
                newDict.TryAdd(taskKey, t);

                _tasks.AddOrUpdate(areaKey, newDict, (k, v) => {
                                                        // An dictionary element if key=areaKey already exists
                                                        // extend and return it.
                                                        v.TryAdd(taskKey, t);
                                                        return v;
                                                       });
            }

            _lastTaskListRefreshDateTime = DateTime.Now;
            return _tasks;
        }

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

Answer 1

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.

Answer 2

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.