Is locking necessary in this ConcurrentDictionary caching scenario

Question

I have the following code to cache instances of some class in a Concurrent Dictionary to which I use in a multi threaded application.

Simply, when I instantinate the class with the id parameter, it first checks if an instance of privateclass with the given id exists in the dictionary, and if not creates an instance of the privateclass (which takes long time, sometimes couple of seconds), and adds it to the dictionary for future use.

public class SomeClass
{
    private static readonly ConcurrentDictionary<int, PrivateClass> SomeClasses =
        new ConcurrentDictionary<int, PrivateClass>();

    private readonly PrivateClass _privateClass;

    public SomeClass(int cachedInstanceId)
    {
        if (!SomeClasses.TryGetValue(cachedInstanceId, out _privateClass))
        {
            _privateClass = new PrivateClass(); // This takes long time
            SomeClasses.TryAdd(cachedInstanceId, _privateClass);
        }
    }

    public int SomeCalculationResult()
    {
        return _privateClass.CalculateSomething();
    }

    private class PrivateClass
    {
        internal PrivateClass()
        {
            // this takes long time
        }

        internal int CalculateSomething()
        {
            // Calculates and returns something
        }
    }
}

My question is, do I need to add a lock around the generation and assignment part of the outer classes constructor to make this code thread safe or is it good as it is?

Update:

After SLaks's suggestion, tried to use GetOrAdd() method of ConcurrentDictionary with the combination of Lazy, but unfortunately the constructor of the PrivateClass still called more than once. See https://gist.github.com/3500955 for the test code.

Update 2: You can see the final solution here: https://gist.github.com/3501446

Answer 1

You're misusing ConcurrentDictionary.

In multi-threaded code, you should never check for the presence of an item, then add it if it's not there.
If two threads run that code at once, they will both end up adding it.

In general, there are two solutions tho this kind of problem. You can wrap all of that code in a lock, or you can redesign it to the whole thing in one atomic operation.

ConcurrentDictionary is designed to for this kind of scenario.

You should simply call

 _privateClass = SomeClasses.GetOrAdd(cachedInstanceId, key => new PrivateClass());

Answer 2

Locking is not necessary, but what you're doing is not thread-safe. Instead of first checking the dictionary for presence of an item and then adding it if necessary, you should use ConcurrentDictionary.GetOrAdd() to do it all in one atomic operation.

Otherwise, you're exposing yourself to the same problem that you'd have with a regular dictionary: another thread might add an entry to SomeClasses after you check for existence but before you insert.