2

I am trying to fix a method that is supposed to be retrieving arbitrary data from a MemoryCache using the built-in GetOrCreateAsync. Here is Microsofts example (from https://learn.microsoft.com/en-us/aspnet/core/performance/caching/memory?view=aspnetcore-3.0) :

public async Task<IActionResult> CacheGetOrCreateAsynchronous()
{
    var cacheEntry = await
        _cache.GetOrCreateAsync(CacheKeys.Entry, entry =>
        {
            entry.SlidingExpiration = TimeSpan.FromSeconds(3);
            return Task.FromResult(DateTime.Now);
        });

    return View("Cache", cacheEntry);
}

Here is mine:

/// <summary>
/// This deadlocks :(
/// </summary>
/// <param name="dayKey"></param>
/// <returns></returns>
public async Task<IEnumerable<Document>> GetEventsForDay(string dayKey)
{
    CheckCacheKeyExists(dayKey);
    return await _cache.GetOrCreateAsync(dayKey, async entry =>
    {
        entry.SetOptions(_cacheEntryOptions);
        var events = await EventsForDay(dayKey).ConfigureAwait(false);
        return events;
    }).ConfigureAwait(false);
}

Where EventsForDay has a signature of

private async Task<IEnumerable<Document>> EventsForDay(string dayKey)

And all async operations within EventsForDay are await and ConfigureAwait(false) to try prevent deadlocks. However even with those ConfigureAwait(false), the async lambda above still deadlocks and never returns.

However, if I use the solution detailed here: https://stackoverflow.com/a/40569410 it no longer deadlocks:

/// <summary>
/// This does not deadlock. Credit here: https://stackoverflow.com/a/40569410
/// </summary>
/// <param name="dayKey"></param>
/// <returns></returns>
public async Task<IEnumerable<Document>> GetEventsForDay(string dayKey)
{
    CheckCacheKeyExists(dayKey);
    var cachedItem = await Task.Run(async () =>
    {
        return await _cache.GetOrCreateAsync(dayKey, async entry =>
        {
            entry.SetOptions(_cacheEntryOptions);
            var events = await EventsForDay(dayKey).ConfigureAwait(false);
            return events;
        }).ConfigureAwait(false);
    }).ConfigureAwait(false);
    return cachedItem;
}

My questions are:

1.) Why does wrapping my async lambda in a Task.Run remove the deadlock?

2.) Why does my async lambda deadlock in the first place if the entire call stack after GetEventsForDay always uses ConfigureAwait(false) for any awaited methods in addition to the lambda itself being awaited with its own ConfigureAwait(false)?

Gabriel Luci
  • 38,328
  • 4
  • 55
  • 84
Giardino
  • 1,367
  • 3
  • 10
  • 30
  • I would imagine `GetOrCreateAsync` calls your expression without `ConfigureAwait(false)` and therefore attempts to resume on the current (blocked) thread. – Johnathan Barclay Nov 26 '19 at 15:58
  • ASP.NET Core doesn't have a synchronization context, so `ConfigureAwait(false)` has no effect: https://blog.stephencleary.com/2017/03/aspnetcore-synchronization-context.html – Gabriel Luci Nov 26 '19 at 16:08
  • 2
    How is `_cacheEntryOptions` declared? And where does it deadlock? Does it deadlock where you're calling `GetEventsForDay(...)`? Or at the `await _cache.GetOrCreateAsync(...)`? – Gabriel Luci Nov 26 '19 at 16:09
  • There is another potential issue here. What type is your `_cache`? If there's a possibility that `GetOrAddAsync()` will call the delegate multiple times, then you'll be launching multiple tasks. `AsyncLazy` will help you avoid that. – Tanveer Badar Nov 28 '19 at 12:36

1 Answers1

2

Great question.

When you do Task.Run, the delegate that you pass to it will be run on the threadpool, but importantly here for you, not with a SynchronizationContext. This is what would usually give you a deadlock, when you are blocking on the result of some code, which itself has exclusive access to that context.

So Task.Run "fixes it", except it doesn't really, it just masks another problem, you are blocking threads on async work. This can eventually lead to thread pool starvation at load, where .NET cannot add any more threads to unblock the current work.

.ConfigureAwait(false) when using await means that if the Task was incomplete then the following code doesn't need to run on the current SynchronizationContext, this also contributes to the deadlock scenario mentioned earlier.

An oversight people often have is that it effectively does nothing when then Task is already complete, for example Task.FromResult, or maybe you have some cached result. In this case the SynchronizationContext flows further into your code, increasing the chance of some subsequent code blocking an async Task with it still present, resulting in a deadlock.

Going back to your example, it doesn't matter how many .ConfigureAwait(false) you put into your code, if the code that runs before the await does some blocking over async code then you are still susceptible to deadlocking. This would also fix it for you, assuming that the source of the deadlock occurs in EventsForDay (don't do this):

public async Task<IEnumerable<Document>> GetEventsForDay(string dayKey)
{
    CheckCacheKeyExists(dayKey);
    var cachedItem = await _cache.GetOrCreateAsync(dayKey, async entry =>
        {
            await Task.Delay(100).ConfigureAwait(false); // this would also stop the deadlock
            entry.SetOptions(_cacheEntryOptions);
            var events = await EventsForDay(dayKey);
            return events;
        });
    return cachedItem;
}

and so would this:

public async Task<IEnumerable<Document>> GetEventsForDay(string dayKey)
{
    CheckCacheKeyExists(dayKey);
    var cachedItem = await _cache.GetOrCreateAsync(dayKey, async entry =>
        {
            SynchronizationContext.SetSynchronizationContext(null); // this would stop it too
            entry.SetOptions(_cacheEntryOptions);
            var events = await EventsForDay(dayKey);
            return events;
        });
    return cachedItem;
}
Stuart
  • 5,358
  • 19
  • 28
  • I do not like the both solutions, can any boddy contribute another idea please? but I do like the theory, which is same as we are educated by pros. Thanks you, anyway. – Dongdong Dec 03 '19 at 17:16
  • No problem, it's good you don't like them, they were to illustrate how this works, hence "don't do this". The fix will be to remove the deadlock scenario from `EventsForDays`, you'd need to share the code for it including any 3rd party code called. – Stuart Dec 03 '19 at 17:38
  • @Stuart what would be "wrong" with doing what you suggested? Or were you saying it would be better to find the root cause of the deadlock vs putting a bandaid on it? – Giardino Dec 17 '19 at 12:03
  • Exactly that, you are working around it here and removing the deadlock, but this could come back to bite you if you want your app to scale. – Stuart Dec 17 '19 at 12:06
  • @Stuart one more question, you said "it effectively does nothing when then Task is already complete, for example Task.FromResult". To clarify, that "complete" task can be anywhere on the call stack right? So even if I have `ConfigureAwait(false)` everywhere I can think of, if _somewhere_ on the call stack there is a `Task.FromResult` or a `Task.Result` that it can deadlock the whole chain? – Giardino Dec 17 '19 at 12:08
  • If there is a `Task.Result` somewhere in the chain then yes it can, if before calling that you do an `await` on a Task with `ConfigureAwait(false)` then you are good, unless that task was already completed (for example using `Task.FromResult`). That's a long way of saying that yes, a combination of this can cause a deadlock. Since posting my answer there is an amazing new .NET blog post that now covers what I'm talking about: https://devblogs.microsoft.com/dotnet/configureawait-faq/ – Stuart Dec 17 '19 at 12:49
  • @Stuart As far as I know, .NET Core does not have SynchronizationContext. Am I missing something? – mnj Jun 09 '20 at 17:20