AddOrUpdate method in ConcurrentDictionary thread safe alternative

Question

As per this link Is AddOrUpdate thread safe in ConcurrentDictionary? from the "Remarks" section on this page https://msdn.microsoft.com/en-us/library/dd287191(v=vs.110).aspx. it says

"However, delegates for these methods are called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, the code executed by these delegates is not subject to the atomicity of the operation."

The issue is I extensively use this throughout my code and realize i have a potential very nasty bug (my understanding has been wrong all along)... since in the way I use the below I did not expect the delegates could be called at the same time and overwrite the internal dictionary:

internal class HandlerProducers
{
    readonly ConcurrentDictionary<Type, Dictionary<Type, InstanceProducer>> handlerProducers
        = new ConcurrentDictionary<Type, Dictionary<Type, InstanceProducer>>();

    public void AddProducer(Type notificationType, Type concreteType, InstanceProducer producer)
    {
        this.handlerProducers
                .AddOrUpdate(
                    notificationType,
                    (key) => new Dictionary<Type, InstanceProducer> { { concreteType, producer } },
                    (key, dictionary) => { dictionary.Add(concreteType, producer); return dictionary; });
    }

    public IEnumerable<InstanceProducer> GetForHandler(Type notificationHandlerType)
    {
        if(this.handlerProducers.TryGetValue(notificationHandlerType, out var dict))
        {
            foreach (var kvp in dict)
                yield return kvp.Value;
        }
    }
}

I have a further challenge that naively putting locks in place will potentially cause a hot spot, the above class is used extensively for reading from via GetForHandler() and is written to occasionally (via the AddProducer() method.

What would be the best way to ensure this is thread safe by design by performant with massive reads and occasional writes?

As I mentioned in your other question, the simplest you can do is to leave `AddOrUpdate` and use `TryAdd` together with `TryUpdate` - those are thread-safe and don't require much change — Camilo Terevinto, Aug 08 '20 at 11:23
Thanks Camilo, you opened my eyes to a nasty bug. I will update my previous question as that is a different use case (similair problem from my misunderstanding but may need to solve with a delayed subscribe I will explain there) and try to answer this one with tryadd as you kindly recommend — morleyc, Aug 08 '20 at 11:28
It is only an issue if you do something thread-unsafe in the lambda expressions. Like using handlerProducers. No issue here, posted code is safe. — Hans Passant, Aug 08 '20 at 12:19

score 2 · Accepted Answer · answered Aug 08 '20 at 11:58

I suggest to use a nested ConcurrentDictionary structure:

readonly ConcurrentDictionary<Type, ConcurrentDictionary<Type, InstanceProducer>> handlerProducers
    = new ConcurrentDictionary<Type, ConcurrentDictionary<Type, InstanceProducer>>();

You could then implement the AddProducer method like this:

public void AddProducer(Type notificationType, Type concreteType,
    InstanceProducer producer)
{
    ConcurrentDictionary<Type, InstanceProducer> innerDict =
        this.handlerProducers.GetOrAdd(notificationType,
            _ => new ConcurrentDictionary<Type, InstanceProducer>());

    if (!innerDict.TryAdd(concreteType, producer))
        throw new InvalidOperationException(
            $"{notificationType}.{concreteType} already exists.");
}

The method GetForHandler needs no modification.

This implementation will make your HandlerProducers class truly thread-safe, without sacrificing its efficiency.

AddOrUpdate method in ConcurrentDictionary thread safe alternative

1 Answers1