Enable-Migrations Exception Calling "Setdata" with "2" Argument(S)

Is ConcurrentDictionary.GetOrAdd truly thread-safe?

To quote the documentation (emphasis mine):

For modifications and write operations to the dictionary, ConcurrentDictionary<TKey,TValue> uses fine-grained locking to ensure thread safety. (Read operations on the dictionary are performed in a lock-free manner.) However, the valueFactory delegate is called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, GetOrAdd is not atomic with regards to all other operations on the ConcurrentDictionary<TKey,TValue> class.

Since a key/value can be inserted by another thread while valueFactory is generating a value, you cannot trust that just because valueFactory executed, its produced value will be inserted into the dictionary and returned. If you call GetOrAdd simultaneously on different threads, valueFactory may be called multiple times, but only one key/value pair will be added to the dictionary.

So while the dictionary is properly thread-safe, calls to the valueFactory, or _ => SampleTask() in your case, are not guaranteed to be unique. So your factory function should be able to live with that fact.

You can confirm this from the source:

public TValue GetOrAdd(TKey key, Func<TKey, TValue> valueFactory)
{
    if (key == null) throw new ArgumentNullException("key");
    if (valueFactory == null) throw new ArgumentNullException("valueFactory");

    TValue resultingValue;
    if (TryGetValue(key, out resultingValue))
    {
        return resultingValue;
    }
    TryAddInternal(key, valueFactory(key), false, true, out resultingValue);
    return resultingValue;
}

As you can see, valueFactory is being called outside of TryAddInternal which is responsible of locking the dictionary properly.

However, since valueFactory is a lambda function that returns a task in your case (_ => SampleTask()), and the dictionary will not await that task itself, the function will finish quickly and just return the incomplete Task after encountering the first await (when the async state machine is set up). So unless the calls are very quickly after another, the task should be added very quickly to the dictionary and subsequent calls will reuse the same task.

If you require this to happen just once in all cases, you should consider locking on the task creation yourself. Since it will finish quickly (regardless of how long your task actually takes to resolve), locking will not hurt that much.

Calling async callback from ConcurrentDictionary - potential deadlock?

Your first approach with ConcurrentDictionary is problematic, because according to the documentation:

If you call AddOrUpdate simultaneously on different threads, addValueFactory may be called multiple times, but its key/value pair might not be added to the dictionary for every call.

[...] The addValueFactory and updateValueFactory delegates are called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, AddOrUpdate is not atomic with regards to all other operations on the ConcurrentDictionary<TKey,TValue> class.

The way you use the ConcurrentDictionary indicates that you expect that the supplied lambdas will be executed at most once for each call to AddOrUpdate, and this is not guaranteed.

Your second approach with a normal Dictionary+lock will not compile, because you are not allowed to await inside a lock block. You must either move the await messageBus.Subscribe(topic); outside of the protected region, or use an asynchronous throttler like the SemaphoreSlim.

ConcurrentDictionary.GetOrAdd - Add only if not null

public User GetUser(int userId)
{
    var user = _user.GetOrAdd(userId, GetUserFromDb);
    if (user == null) _user.TryRemove(userId, out user);    
}

You can also wrap that into an extension method:

public static TValue GetOrAddIfNotNull<TKey, TValue>(
    this ConcurrentDictionary<TKey, TValue> dictionary,
    TKey key, 
    Func<TKey, TValue> valueFactory) where TValue : class
{
    var value = dictionary.GetOrAdd(key, valueFactory);
    if (value == null) dictionary.TryRemove(key, out value);
    return value;
}

Then your code will look like:

public User GetUser(int userId)
{
    var user = _user.GetOrAddIfNotNull(userId, GetUserFromDb)   
}

UPDATE

As per @usr comment, there might be a case when:

1. Thread 1 executes GetOrAdd, adds null to the dictionary and pauses.
2. User is added to the database.
3. Thread 2 executes GetOrAdd and retrieves null from the dictionary instead of hitting the database.
4. Thread 1 and Thread 2 execute TryRemove and remove record from the dictionary.

With this timing, Thread 2 will get null instead of hitting the database and getting the user record. If this edge case matters to you and you still want to use ConcurrentDictionary, then you can use lock in the extension method:

public static class ConcurrentDictionaryExtensions
{
    private static readonly object myLock = new object();

    public static TValue GetOrAddIfNotNull<TKey, TValue>(
        this ConcurrentDictionary<TKey, TValue> dictionary,
        TKey key, 
        Func<TKey, TValue> valueFactory) where TValue : class
    {
        lock (myLock)
        {
            var value = dictionary.GetOrAdd(key, valueFactory);
            if (value == null) dictionary.TryRemove(key, out value);
            return value;
        }
    }
}

---

Related Topics