Deadlock with ContinueWiths in WebAPI

问题:

We've been running into a lot of deadlocks as part of exposing some of existing code over Web API. I've been able to distill the problem down to this very simple example that will hang forever:

public class MyController : ApiController
{
    public Task Get()
    {
        var context = TaskScheduler.FromCurrentSynchronizationContext();

        return Task.FromResult(1)
            .ContinueWith(_ => { }, context)
            .ContinueWith(_ => Ok(DateTime.Now.ToLongTimeString()), context);
    }
}

To me, that code seems simple enough. This might seem a bit contrived but that's only because I've tried simplifying the problem as much as possible. It seems having two ContinueWiths chained like this will cause the deadlock - if I comment out the first ContinueWith (that isn't really doing anything anyway), it will work just fine. I can also 'fix' it by not giving the specific scheduler (but that isn't a workable solution for us since our real code needs to be on the correct/original thread). Here, I've put the two ContinueWiths right next to each other but in our real application there is a lot of logic that is happening and the ContinueWiths end up coming from different methods.

I know I could re-write this particular example using async/await and it would simplify things and seems to fix the deadlock. However, we have a ton of legacy code that has been written over the past few years - and most of it was written before async/await came out so it uses ContinueWith's heavily. Re-writing all that logic isn't something we'd like to do right now if we can avoid it. Code like this has worked fine in all the other scenarios we've run into (Desktop App, Silverlight App, Command Line App, etc.) - it's just Web API that is giving us these problems.

Is there anything that can be done generically that can solve this kind of deadlock? I'm looking for a solution that would hopefully not involve re-writing all ContinueWith's to use async/await.

Update:

The code above is the entire code in my controller. I've tried to make this reproducible with the minimal amount of code. I've even done this in a brand new solution. The full steps that I did:

1. From Visual Studio 2013 Update 1 on Windows 7 (with .NET Framework 4.5.1), create a new Project using the ASP.NET Web Application Template
2. Select Web API as the template (on the next screen)
3. Replace the Get() method in the auto-created ValuesController with the example given in my original code
4. Hit F5 to start the app and navigate to ./api/values - the request will hang forever
5. I've tried hosting the web site in IIS as well (instead of using IIS Express)
6. I also tried updating all the various Nuget packages so I was on the latest of everything

The web.config is untouched from what the template created. Specifically, it has this:

<system.web>
   <compilation debug="true" targetFramework="4.5" />
   <httpRuntime targetFramework="4.5" />
</system.web>

回答1:

Try the following (untested). It's based on the idea that AspNetSynchronizationContext.Send executes the callback synchronously and thus should not result in the same deadlock. This way, we enter the AspNetSynchronizationContext on a random pool thread:

public class MyController : ApiController
{
    public Task Get()
    {
        // should be AspNetSynchronizationContext
        var context = SynchronizationContext.Current;

        return Task.FromResult(1)
            .ContinueWith(_ => { }, TaskScheduler.Default)
            .ContinueWith(_ =>
            {
                object result = null;
                context.Send(__ => { result = Ok(DateTime.Now.ToLongTimeString()); }, 
                    null);
                return result;
            }, TaskScheduler.Default);
    }
}

Updated, based on the comments, apparently it works and eliminates the deadlock. Further, I'd build a custom task scheduler on top of this solution, and use it instead of TaskScheduler.FromCurrentSynchronizationContext(), with very minimal changes to the existing code base.

回答2:

Based on Noseratio's answer, I came up with the following 'Safe' versions of ContinueWith. When I update my code to use these safe versions, I don't have anymore deadlocks. Replacing all my existing ContinueWiths with these SafeContinueWiths probably won't be too bad....it certainly seems easier and safer than re-writing them to use async/await. And when this executes under non-ASP.NET contexts (WPF App, Unit Tests, etc.), it will fall back to the standard ContinueWith behavior so I should have perfect backwards compatability.

I'm still not sure this is the best solution. It seems like this is a pretty heavy-handed approach that is necessary for code that seems so simple.

With that said, I'm presenting this answer in case it triggers a great idea from somebody else. I feel like this can't be the ideal solution.

New controller code:

public Task Get()
{
    return Task.FromResult(1)
               .SafeContinueWith(_ => { })
               .SafeContinueWith(_ => Ok(DateTime.Now.ToLongTimeString()));
}

And then the actual implementation of SafeContinueWith:

public static class TaskExtensions
{
    private static bool IsAspNetContext(this SynchronizationContext context)
    {
        //Maybe not the best way to detect the AspNetSynchronizationContext but it works for now
        return context != null && context.GetType().FullName == "System.Web.AspNetSynchronizationContext";
    }

    /// <summary>
    /// A version of ContinueWith that does some extra gynastics when running under the ASP.NET Synchronization 
    /// Context in order to avoid deadlocks.  The <see cref="continuationFunction"/> will always be run on the 
    /// current SynchronizationContext so:
    /// Before:  task.ContinueWith(t => { ... }, TaskScheduler.FromCurrentSynchronizationContext());
    /// After:   task.SafeContinueWith(t => { ... });
    /// </summary>
    public static Task<T> SafeContinueWith<T>(this Task task, Func<Task,T> continuationFunction)
    {
        //Grab the context
        var context = SynchronizationContext.Current;

        //If we aren't in the ASP.NET world, we can defer to the standard ContinueWith
        if (!context.IsAspNetContext())
        {
            return task.ContinueWith(continuationFunction, TaskScheduler.FromCurrentSynchronizationContext());
        }

        //Otherwise, we need our continuation to be run on a background thread and then synchronously evaluate
        //  the continuation function in the captured context to arive at the resulting value
        return task.ContinueWith(t =>
        {
            var result = default(T);
            context.Send(_ => result = continuationFunction(t), null);
            //TODO: Verify that Send really did complete synchronously?  I think it's required to by Contract?
            //      But I'm not sure I'd want to trust that if I end up using this in producion code.
            return result;
        });
    }

    //Same as above but for non-generic Task input so a bit simpler
    public static Task SafeContinueWith(this Task task, Action<Task> continuation)
    {
        var context = SynchronizationContext.Current;
        if (!context.IsAspNetContext())
        {
            return task.ContinueWith(continuation, TaskScheduler.FromCurrentSynchronizationContext());
        }

        return task.ContinueWith(t => context.Send(_ => continuation(t), null));
    }
}

回答3:

You can set TaskContinuationOptions.ExecuteSynchronously:

return Task.FromResult(1)
    .ContinueWith(_ => { }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, context)
    .ContinueWith(_ => Ok(DateTime.Now.ToLongTimeString()), CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, context);

There is also a "global" way to get this to work; in your web.config, add this to your appSettings:

<add key="aspnet:UseTaskFriendlySynchronizationContext" value="false" />

However, I can't really recommend the global approach. With that appsetting, you can't use async/await in your application.