Improving handling of data structure used in parallel

Question

I have a class extending the Dictionary class. This class is used for storing some information (modeled in CustomClass) and accessing it through an integer ID.

To extend this class I have added a TryAdd method, specific to my workflow, which implement different behaviours for the cases of trying to add a new or a already existing CustomClass object.

public class CustomDictionary : Dictionary<int, CustomClass>
    {
        private void TryAdd(int ID, CustomClass customObject)
        {
            if (this.ContainsKey(ID))
            {
                //some operations
            }
            else
            {
                //some others operations
                this.Add(customObject);
            }
        }
    }

There will be only one instance object for this class. I want to add locks to provide thread safety and syncronisation for the object of type CustomDictionary. TryAdd and data accessing operation will frequently occur in parallel for this structure.

Have in mind that at the moment I can't use framework 4.0 so I can't use ConcurrentCollections.

To ensure thread safety I put this.Add(customObject); within a lock(this) but I have read that this is very bad.

Then I read about locking using a private object.

public class CustomDictionary : Dictionary<int, CustomClass>
    {
        private object lockObject = new Object();

        private void TryAdd(int ID, CustomClass customObject)
        {
            if (this.ContainsKey(ID))
            {
                //some operations
            }
            else
            {
                //some others operations
                lock(lockObject)
                {
                    this.Add(customObject);
                }
            }
        }
    }

Is this the good way of doing it? Should I also lock the CustomDictionary object when I read data from that object?

Any improvements for the implementation of this class would be helpfull.

Answer 1

First of all, it's very good that you read about the evilness of lock(this). Avoid it at all costs :)

As for your solution, it's not thread safe, in terms of functionality:

Thread A tries to add an object to id = 123.
Since this object doesn't exist, the ContainsKey returns false, so the false part of the condition takes place.
Then Thread A acquires the lock and starts to add its object.

However, Thread A hasn't yet completed its processing, and the system now switches to Thread B.

Strangely enough, Thread B wants to add its own object with the same id = 123 (!).

And, what do you know, since Thread A hasn't finished its processing, Thread B asks if the dictionary ContainsKey(123) and gets false. That's the problem.

Even worse, when Thread B starts its processing of adding the object, an exception would be thrown, since there's already a key assigned with 123.

So, no, the suggested code is not thread safe. That's a classic race condition.

How to resolve this?

Option 1: Double check on read operations

Inside the lock, you can call again to the ContainsKey, which is a read operation. MS provided a cheering statement, that read operations on its data structures (dictionary, list, etc.) are all thread safe.

Update: from MSDN: A Dictionary can support multiple readers concurrently, as long as the collection is not modified (my emphasis). So please ignore this option.

Option 2: Use ReaderWriterLockSlim Class

This class is optimized to the scenario of multiple reads / seldom writes. So when you want to write, you upgrade your lock to have "write" scope, and you continue your code.

Option 3: Use a simple lock on the entire TryAdd method. This is very straight forward.

All options are fine, and my personal bias is towards option 2.

Update: see also a related post at Ayende: Why is this not thread safe?

Good luck!