C# Multiple ASYNC requests single response (Promise like)

Question

I'm no too good with async processes in C#. I need to implement method, that can be called multiple times by async requests, but request data and serve response once. I've created this method, inspired by common JS promise pattern:

private Task<bool> DoSomething(string key)
{
    // generating unique key
    string cacheKey = "do-comething-" + key;
    // searching cache for a Task in progress
    var cachedRequest = MemoryCache.Default.Get(cacheKey, null) as Task<bool>;

    // if found - return old Task
    if (cachedRequest != null)
    {
        return cachedRequest;
    }

    TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>();

    CacheItemPolicy cip = new CacheItemPolicy()
    {
        AbsoluteExpiration = new DateTimeOffset(DateTime.Now.AddSeconds(60))
    };

    // storing Task object in cache
    MemoryCache.Default.Set(cacheKey, tcs.Task, cip);

    try
    {
        // do something long here    
        tcs.TrySetResult(true);
    }
    catch(Exception ex)
    {
        tcs.TrySetResult(false);
    }
    finally
    {
        MemoryCache.Default.Remove(cacheKey);
    }
    return tcs.Task;
}

Is it ok, or I'm totally out of it? Any advice will be highly appreciated. Thanks.

Answer 1

Bug

You have a pretty big multithreading issue in your code. When this method can be called from different threads, the MemoryCache doesn't necessarily contain an already created task. Consider the following execution:

Thread 1                     | Thread 2
string cacheKey = ...        |  -
var cachedRequest = ..       |  -
if (cachedRequest != null)   |  -
if (cachedRequest != null)   |  string cacheKey = ...
if (cachedRequest != null)   |  var cachedRequest = ...
TaskCompletionSource tcs =   |  var cachedRequest = ...
TaskCompletionSource tcs =   |  if (cachedRequest != null) 
TaskCompletionSource tcs =   |  TaskCompletionSource tcs = 
...                          |  ...

The result should be pretty obvious: This way two tasks for the same key can be started. Depending on how strict you need that semantic, that's a problem.

Fixing this is not trivial, but should be simple enough with a lock (_lock) block onto a private static const Object _lock = new Object();

How to Task

In general I'd expect a method that returns a Task<bool> to not actually do the work, as is suggested by //do something long here. Instead the code should submit the work as an Action or something along these lines. Something like that:

Task<bool> result = new Task<bool>(() => 
{
    try
    {
         // do something long here
         return true;
    }
    catch (Exception e) 
    {
         return false;
    }
});

this shows an additional problem. The return value of that Task is basically just a flag for successful completion. But you know what? Task already has that: Task.IsFaulted

This makes for a pretty different return value. Use Task instead of Task<bool>, because it removes redundancy.

Back to the code: We're still not quite there with the lookup and caching mechanics.

CacheItemPolicy cip = new CacheItemPolicy()
...
MemoryCache.Default.Set(cacheKey, result, cip);

This is for the caching, now we just need to clean the cache when we're finished:

result.ContinueWith(() => MemoryCache.Default.Remove(cacheKey));
result.Start(); // actually start doing work
return result;