CODEWITHSUNDEEP
c#multithreadingconcurrency
M. Mennan Kara
M. Mennan Kara

Reputation: 10222

Is locking necessary in this ConcurrentDictionary caching scenario

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

Upvotes: 10

Views: 14857

Answers (3)

Trondster
Trondster

Reputation: 116

Your sample code at https://gist.github.com/3500955 using ConcurrentDictionary and Lazy<T> is incorrect - you're writing:

    private static readonly ConcurrentDictionary<int, PrivateClass> SomeClasses =
        new ConcurrentDictionary<int, PrivateClass>();
    public SomeClass(int cachedInstanceId)
    {
        _privateClass = SomeClasses.GetOrAdd(cachedInstanceId, (key) => new Lazy<PrivateClass>(() => new PrivateClass(key)).Value);
    }

..which should have been:

    private static readonly ConcurrentDictionary<int, Lazy<PrivateClass>> SomeClasses =
        new ConcurrentDictionary<int, Lazy<PrivateClass>>();
    public SomeClass(int cachedInstanceId)
    {
        _privateClass = SomeClasses.GetOrAdd(cachedInstanceId, (key) => new Lazy<PrivateClass>(() => new PrivateClass(key))).Value;
    }

You need to use ConcurrentDictionary<TKey, Lazy<TVal>>, and not ConcurrentDictionary<TKey, TVal>. The point is that you only access the Value of the Lazy after the correct Lazy object has been returned from the GetOrAdd() - sending in the Value of the Lazy object to the GetOrAdd function defeats the whole purpose of using it.

Edit: Ah - you got it in https://gist.github.com/mennankara/3501446 :)

Upvotes: 3

ikh
ikh

Reputation: 2436

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.

Upvotes: 7

SLaks
SLaks

Reputation: 887519

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());

Upvotes: 16

Related Questions