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;
    }
// ...

I believe just using `ConfigureAwait(false)` (for the `client.SendAsync` call) would resolve the problem. The provided solution is just terrible (and must not pass the code review). Further reading: https://msdn.microsoft.com/en-us/magazine/jj991977.aspx — zerkms, Sep 12 '16 at 00:17
Sorry I copied the code wrongly. The `ConfigureAwait(false)` is currently included. I have updated the current example. Sorry about that. — Linvi, Sep 12 '16 at 00:45
`httpResponseMessage = _httpClientWebHelper.GetHttpResponse(twitterQuery, handler).Result;` — Linvi, Sep 12 '16 at 00:48
And what method this call is placed in? Is it not `async`? And cannot be turned into one? — zerkms, Sep 12 '16 at 00:49
The method is not async and I cannot (for now) change the entire call tree to make all of the methods async. You can look at the code on github if you want more context : https://github.com/linvi/tweetinvi/blob/master/Tweetinvi.WebLogic/WebRequestExecutor.cs and https://github.com/linvi/tweetinvi/blob/master/Tweetinvi.WebLogic/HttpClientWebHelper.cs — Linvi, Sep 12 '16 at 00:51
Same for me. I do not understand how this code could cause a deadlock... Even if the TaskScheduler runs out of available threads, it should simply wait for one to be free. Am I correct? — Linvi, Sep 12 '16 at 01:16
That's what my understanding of the `async/await` and the threading model tells me as well. — zerkms, Sep 12 '16 at 01:25
@Linvi the "fix" guarantees a blocking call (.Result) but wastes a Task to do it. Are you sure you have a deadlock at all? Most likely your code is making more than 2 requests against the same server and hits the 2 requests per server limit. The solution is to increase the limit — Panagiotis Kanavos, Sep 12 '16 at 11:37
I will look into this and get more information from the user's configuration. Thanks for the info. — Linvi, Sep 12 '16 at 14:21

Mrinal Kamboj · Answer 1 · 2016-09-12T11:31:13.793

1

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

It has converted the Async call to Sync call, It is a blocking call due to usage of Result on the Task
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();
  }
}

edited Sep 12 '16 at 11:31

answered Sep 12 '16 at 05:12

Mrinal Kamboj

11,300
5
40
74

Thank you, I will take a better look at your solution/explanation and get back to you. – Linvi Sep 12 '16 at 09:50
Check edited answer, it suggests why `ConfigureAwait(false)` is leading to deadlock – Mrinal Kamboj Sep 12 '16 at 11:22
Could you provide more insight. Am I misusing the ConfigureAwait here? – Linvi Sep 12 '16 at 11:24
Not misuse but incorrect use, since I remember talking to Stephen Cleary on this and he suggested the whole chain needs to be using the `ConfigureAwait(false)`, till the very end to dispatch the I/O call, else it cannot ignore the Synchronization context – Mrinal Kamboj Sep 12 '16 at 11:26
Check another way to make the main caller `Synchronous`, which is not as preferred as whole thing can be Asynchronous – Mrinal Kamboj Sep 12 '16 at 11:33
I remember reading about the entire chain to avoid using the `SynchronizationContext`. But I cannot do that without a huge refactoring. But what would you suggest when you say `Check another way to make the main caller Synchronous`. – Linvi Sep 12 '16 at 14:37
Posted the code before adding the comment, check: `Make your code Synchronous as follows (now you don't need ConfigureAwait(false) in whole chain):` – Mrinal Kamboj Sep 12 '16 at 15:55
I will try it out and check what the `GetAwaiter` does compared to the await. Thanks for the help. – Linvi Sep 12 '16 at 16:03
`return await client.SendAsync(new HttpRequestMessage());` if you do not specify the `.ConfigureAwait`, the `SynchronizationContext` will be the one of the caller which could be a UI thread? – Linvi Sep 12 '16 at 16:05
You intend to use only `.GetAwaiter().GetResult()`, in my understanding that may not work, but please check it out, not 100% sure. `ConfigureAwait(false)` might be required in this case. – Mrinal Kamboj Sep 12 '16 at 16:08
1

Also check this question, it is listing an interesting option, though no answers yet http://stackoverflow.com/questions/39447571/async-library-best-practice-configureawaitfalse-vs-setting-the-synchronizati – Mrinal Kamboj Sep 12 '16 at 16:11
" Point is client.SendAsync and complete call chain needs to have a ConfigureAwait(false)" --- and they don't have it? So, uhm, you're assuming microsoft has implemented their async client incorrectly? – zerkms Sep 12 '16 at 20:22
No they didn't but its not in our control to use `ConfigureAwait(false)` in the whole chain when an external Async method is called, till the point final IO call is dispatched – Mrinal Kamboj Sep 13 '16 at 05:01
@MrinalKamboj it is never in our control actually then, unless you're writing unmanaged code. I'd say suggesting to not trust microsoft is a poor advice. And your current answer is based on an assumption, that MS made a mistake. – zerkms Sep 13 '16 at 21:59
@zerkms It's easy to confound the issue. Never did I suggested, don't trust MS, that's your derivation. I have only suggested correct usage of `ConfigureAwait(false)`, I don't even see a rationale of using it in this case, that's what my preferred solution reflect. For `ConfigureAwait(false)` to work it surely has to be part of complete chain, else it will lead to deadlock, its infact better to make whole call chain async-await compliant – Mrinal Kamboj Sep 15 '16 at 04:00
Check the following links too: Check the links: http://stackoverflow.com/questions/28776408/c-sharp-async-await-chaining-with-configureawaitfalse http://stackoverflow.com/questions/28305968/use-task-run-in-synchronous-method-to-avoid-deadlock-waiting-on-async-method/28307965#28307965 – Mrinal Kamboj Sep 16 '16 at 09:17

score 1 · Answer 2 · answered Sep 12 '16 at 08:59

1

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.

answered Sep 12 '16 at 08:59

shay__

3,815
17
34

Hello, I have added the caller method to the question. Effectively the caller has a `.Result`. As you noticed I do use the `.ConfigureAwait` but I do not understand your last statement mentioning that I should have my own HttpClient interface ensure the `.ConfigureAwait` Isn't it what I am exactly doing, or am I misunderstanding the solution you propose. – Linvi Sep 12 '16 at 09:49
`.Result` means "block until the task finishes then return the result". To avoid this block, you need to use `Task.Run` or the older `TaskFactory.StartNew`. After that of course you need another `.Result`. The result is a blocking call, exactly the same as `SendAsync(..).Result`, only slower. HttpClient doesn't have any concurrency bugs. The line with the double ` .Result` though is definitely buggy. – Panagiotis Kanavos Sep 12 '16 at 11:32
@Linvi were you trying to execute more than 2 requests against the same URL or the same server? This is a TCP/IP issue, *not* related to asynchronous programming. – Panagiotis Kanavos Sep 12 '16 at 11:35
@Panagiotis 1: I agree with you that the double `.Result` is a very strange pattern. I am still reading more on subject and I will receive the OReilly book on `async` tonight. 2: I am not sure if the user is trying to do that but it could be the case. I could come back with more info. – Linvi Sep 12 '16 at 14:16

Linvi · Answer 3 · 2016-09-13T23:39:31.840

1

@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 :)

edited Sep 13 '16 at 23:39

answered Sep 13 '16 at 23:18

Linvi

2,077
1
14
28

So, well, could you reproduce a deadlock in the very first place? If not - how do you know it solved it? If not - how do you know this was the reason caused it? Deadlock with `async/await` and `.Result` is not a random thing: it either reproduces every time or does not exist at all. – zerkms Sep 14 '16 at 00:32
I am waiting for the user who mentioned the bug to reply. But whilst he does not reply, I am trying to understand it. And from my readings, I found out that the `.ConfigureAwait` could actually decide to continue with the same `SynchronizationContext` as the one invoking the `await` function. This is why this solution is making an attempt to force the execution to use a thread from the ThreadPool. – Linvi Sep 14 '16 at 00:39
"could actually decide to continue with the same SynchronizationContext" --- yes, if the "asynchronous" execution could be optimised out. Not your case. And even if it was - it would not lead to a deadlock (because the whole execution would be synchronous). – zerkms Sep 14 '16 at 00:40
Well yes it would as you have the `.Result` blocking on Thread1 and the callback of the Task running in the same SynchronizationContext Thread1. The callback can never return as Thread1 is already waiting for `.Result`. – Linvi Sep 14 '16 at 00:43
Nope. If it could be optimised out - the Task would complete by the moment you access its `Result`, it would not be any other thread involved. – zerkms Sep 14 '16 at 00:43
This task cannot be optimized (its a HttpRequest) and will run on a separate thread. When I say optimize, I am talking about the `SynchronizationContext` of the callback (the one "defined" by `.ConfigureAwait`), not the `SynchronizationContext` of the Task itself. – Linvi Sep 14 '16 at 00:46
So, if it cannot be optimised-out - then the task continuation is guaranteed to run on the other thread. If it does not (and you can prove it) - you must report a .NET CLR/BCL bug. – zerkms Sep 14 '16 at 00:47
Are you saying that if a Task, cannot be optimized, the callback `SynchronizationContext` (for a Task with `.ConfigureAwait`) will always be on another Thread than the caller thread? – Linvi Sep 14 '16 at 00:48
If the thread from a thread pool is blocked (via a `.Result`) - the other task cannot be allocated to run on it (unless you do that explicitly, and you don't with .the help of `ConfigureAwait`) – zerkms Sep 14 '16 at 00:49
Alright, thanks for the info. I will push the user to reply to me and test this solution as soon as I can on his environment. – Linvi Sep 14 '16 at 00:50
Well, I personally would start from the beginning: you must be able to reproduce something to fix it. – zerkms Sep 14 '16 at 00:51
1

I know, and this is always what I try to do. The problem with this one is that this user is not responsive since he mentioned his fix. I might end up leaving the code as is, until the point when he replies (if he does...). Thanks anyway. – Linvi Sep 14 '16 at 00:53
I was thinking and reading of the problem for another good day. And you know what: now I see why neither of your latest or initial solutions should go to production. That's because - if you invoke an async method with `await` - you're always fine. But if you use blocking `.Result` or `.Wait()` - it's your responsibility as a caller to mitigate a potential deadlock. Which leads us to a conclusion: even if it was a deadlock here (I still don't believe it), it must be solved in the call-side, not in the `GetHttpResponse` method implementation. Thoughts? – zerkms Sep 14 '16 at 21:05

score 0 · Answer 4 · edited Sep 14 '18 at 21:39

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.