1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 May 23 '17 at 11:27
  • \$\begingroup\$ @Vogel612, but how can I add details for my question if it's not completely answered? \$\endgroup\$ – Kindzoku May 23 '17 at 11:31
  • \$\begingroup\$ The correct solution to that is to add enough details the first time around :) Since that's not possible anymore, the next best solution is to ask a "follow-up" question. Please make sure to include appropriate context to make sure that your question isn't closed as "hypothetical code" \$\endgroup\$ – Vogel612 May 23 '17 at 11:48
  • \$\begingroup\$ Well, you helped me with main idea. I'll mark an answer as answer and ask my question on SO. Was my error in the first place to post question here. But well, I flagged my question for transition to SO even before your answer. \$\endgroup\$ – Kindzoku May 23 '17 at 12:16
2
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Thx, I'll try to implement your solution. It looks very clear. A lot clearer than mine. \$\endgroup\$ – Kindzoku May 23 '17 at 10:11
  • \$\begingroup\$ I've updated my question with your solution. Do I implemented it right? Feel myself a bit confused. How can I use return value? The method that call DoSomething is sync. How can I get my flag outside? Thx \$\endgroup\$ – Kindzoku May 23 '17 at 11:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.