1

I have a private ConcurrentDictionary that is a simple lookup table of some DB keys.

I'm trying to leverage the ConcurrentDictionary so that it will only do one call to the db when 2+ requests to the same line of code, are made at the same time. (Which is why i'm using a ConcurrentDictionary.)

How can I do this please?

This is what I was trying to do .. but I think it's storing the Task in the dictionary ... not the result of the task....

private readonly ConcurrentDictionary<string, Task<int>> _myKeys = new ConcurrentDictionary<string, Task<int>>();

...

private async Task<int> DoStuffAsync(string key)
{
   // do stuff here.

   return await _myKeys.GetOrAdd(key,
                                 async k => await _db.GetId(k)
                                                     .ConfigureAwait(false))
                       .ConfigureAwait(false);
}

Any ideas?

EDIT:

Notice my method signature and what I'm returning. Is it better to return an int and not a Task<int> and then somehow refactor my db call to still be async .. but .. better?

Pure.Krome
  • 84,693
  • 113
  • 396
  • 647

1 Answers1

2

GetOrAdd does not guarantee that the delegate will be called only once when it's called from multiple threads at the same time with the same value:

If you call GetOrAdd 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.

This can be also seen in the implementation:

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

So, to do about as good a job as GetOrAdd(), you can do something like (input checking omitted):

public static async Task<TValue> GetOrAddAsync<TKey, TValue>(
    this ConcurrentDictionary<TKey, TValue> dictionary,
    TKey key, Func<TKey, Task<TValue>> valueFactory)
{
    TValue resultingValue;
    if (dictionary.TryGetValue(key, out resultingValue))
    {
        return resultingValue;
    }
    return dictionary.GetOrAdd(key, await valueFactory(key));
}

If the requirement to not call the delegate twice at the same time is just a performance optimization, this should be sufficient.

If it's required for correctness of your code, then even GetOrAdd is not sufficient and you will need to use additional synchronization.

svick
  • 236,525
  • 50
  • 385
  • 514
  • 2
    I suspect this is worse than the OP's code for typical cases: in your version, suppose the task takes five seconds, and after three seconds, another task calls `GetOrAddAsync` for the same key. In your code, this is guaranteed to start a new task. In the OP's code, it's extremely likely to re-use the existing task. Yes, you're right that the OP's code doesn't guarantee anything, but it does make it so much more likely that I'd go with the OP's version. –  Oct 17 '16 at 16:50
  • @hvd You're right. I was working under the assumption that you don't want to store `Task`s in the dictionary. – svick Oct 17 '16 at 16:55