Encapsulating async method in Task.Factory.StartNew

Question

I have a good understanding of Task and the async/await pattern but recently someone fixed a deadlock that he said was caused by :

public async Task<HttpResponseMessage> GetHttpResponse()
{
    using (var client = new HttpClient())
    {
        return await client.SendAsync(new HttpRequestMessage()).ConfigureAwait(false);
    }
}

He says that he fixed it with some kind of Task.Factory.StartNew pattern.

public HttpResponseMessage GetHttpResponse()
{
    using (var client = new HttpClient())
    {
        return Task.Factory.StartNew(() => client.SendAsync(new HttpRequestMessage()).Result).Result;
    }
}

First thing is that troubles is me is why would he change the return statement to be an HttpResponseMessage as opposed to a Task<HttpResponseMessage>.

My second question, is why would this code solve a deadlock bug. From my understanding, the fact that he calls a .Result forces the GetHttpResponse Thread to wait (and freeze) until the client.SendAsync completes.

Can anyone try to explain me how this code affects any TaskScheduler and SynchronizationContext.

Thank you for your help.

EDIT : Here is the caller method to provide more context to the problem

public IWebRequestResult ExecuteQuery(ITwitterQuery twitterQuery, ITwitterClientHandler handler = null)
{
    HttpResponseMessage httpResponseMessage = null;

    try
    {
        httpResponseMessage = _httpClientWebHelper.GetHttpResponse(twitterQuery, handler).Result;

        var result = GetWebResultFromResponse(twitterQuery.QueryURL, httpResponseMessage);

        if (!result.IsSuccessStatusCode)
        {
            throw _exceptionHandler.TryLogFailedWebRequestResult(result);
        }

        var stream = result.ResultStream;

        if (stream != null)
        {
            var responseReader = new StreamReader(stream);
            result.Response = responseReader.ReadLine();
        }

        return result;
    }
// ...

Answer 1

I think adding ConfigureAwait to your Note(2) code would have made it work. The ASP.NET context only allows 1 thread to run in it at a time. The Thread pool context allows more than 1 thread at a time to run.

The await returns code execution back to the calling method. The calling method is on the ASP.net context and the call to .Result blocks. When client.SendAsync returns it goes to finish the method on the ASP.net context but can't because there is already a thread in the context blocking (for .Result).

If you used ConfigureAwait with client.SendAsync, it would be able to finish the method in a different context than the ASP.Net context and finish off the method to get to .Result in the calling method.

i.e. .Result is waiting for the end of the GetHttpResponse method to finish while the end of the GetHttpResponse method is waiting for. Result to get out of the ASP.net context to finish.

In your original example it was .Result in the calling method blocking while client.SendAsync was waiting for .Result to get out of the ASP.net context to continue. Like you said ConfigureAwait(false) only affects the context on the code executing from the return after await. While Client.SendAsync is waiting to get into the ASP.NET context to execute.

Answer 2

@mrinal-kamboj @panagiotis-kanavos @shay

Thank you all for your help. As mentioned I have started to read the Async in C# 5.0 from Alex Davies.

What I found

In Chapter 8 : Which Thread Runs My Code, he mentions our case and I think I found an interesting solution there :

You can get around the deadlock problem by moving to the thread pool before starting the async code, so that the SynchronizationContext captured is the thread pool rather than the UI thread.

var result = Task.Run(() => MethodAsync()).Result;

By calling Task.Run he actually forces the async call SynchronizationContext to be the SynchronizationContext of a ThreadPool (which makes sense). By doing so he also insures that the code MethodAsync is neither started nor returned to the main thread.

My solution

By taking this into consideration I changed my code as followed :

public HttpResponseMessage GetHttpResponse()
{
    using (var client = GetHttpClient())
    {
        return TaskEx.Run(() => client.SendAsync(new HttpRequestMessage())).Result;
    }
}

This code seems to work properly for Console, WPF, WinRT and ASP.NET. I will conduct further testing and update this post.

Questions

• Do you think this new code makes sense?
• Do you think it will prevent any potential deadlock?

NOTE

In the book, I learnt that .ConfigureAwait(false) only prevents the callback to call the SynchronizationContext.Post() method to be run on the caller Thread. To determine the thread the callback should run on, the SynchronizationContext check if the thread it is associated with is important. If it is, then it picks another thread.

From my understanding it means that the callback can be run on any thread (UI-Thread or ThreadPool). Therefore it does not guarantee a non-execution on the UI-Thread but makes it very unlikely.

NOTE (2)

It is interesting to note that the following code does not work :

public async Task<HttpResponseMessage> GetHttpResponse()
{
    using (var client = GetHttpClient())
    {
        return await TaskEx.Run(() => client.SendAsync(new HttpRequestMessage()));

When I attempted to have this code I had in mind that the the .Result could be used outside of the scope of the ThreadPool awaited .Result. To some extent it makes sense to me but if any one wants to comment on this too, he will be welcome :)

Answer 3

Modified code to fix the deadlock is very poor usage of Task APIs, I can see so many issues there:

1. It has converted the Async call to Sync call, It is a blocking call due to usage of Result on the Task
2. Task.Factory.StartNew, is a big No, check StartNew is Dangerous by Stephen Cleary

Regarding your original code, following is the most probable reason for deadlock:

• Reason why ConfigureAwait(false) doesn't work is you need to use it in complete Call stack in all the Async calls, that's why it is leading to Deadlock. Point is client.SendAsync and complete call chain needs to have a ConfigureAwait(false), for it to ignore the Synchronization context. It doesn't help to be just in one place and there's no guarantee for calls which are not in your control

Possible Solution:

• Here the simple removal shall work, you don't even need Task.WhenAll, as there are no multiple tasks

   using (var client = new HttpClient())
  {
      return await client.SendAsync(new HttpRequestMessage());
  }
  

Another not so preferred option would be:

• Make your code Synchronous as follows (now you don't need ConfigureAwait(false) in whole chain):

  public HttpResponseMessage GetHttpResponse()
  {
     using (var client = new HttpClient())
     {
         return await client.SendAsync(new    
           HttpRequestMessage()).ConfigureAwait(false).GetAwaiter().GetResult();
    }
  }
  

Answer 4

The deadlock wasn't caused by return await client.SendAsync(new HttpRequestMessage()).ConfigureAwait(false);. It was caused due to a blocking call down the stack. Since it is very unlikely that MS implementation of HttpClient.SendAsync() has some blocking code inside (which might deadlock), it must be one of the callers to public async Task<HttpResponseMessage> GetHttpResponse() that use .Wait() or .Result on the returned Task. All your colleague did is to move the blocking call up the stack, where it is more visible. Further more, this "fix" does not even solve the classic deadlock since it use the same synchronization context(!). You can deduce that somewhere else down the stack some other function offloads a new Task without asp.net synchronization context, and in that new (probably default) context your blocking GetHttpResponse() executes, otherwise the "fix" would have deadlock too!

Since in the real world it is not always possible to refactor production legacy code to be async all the way, you should use your own interface of async HttpClient, and make sure the implementation use .ConfigureAwait(false), as all infrastructure libraries should.