1

I found this method:

public override void OnActionExecuting(HttpActionContext actionContext)
{
    var body = Task.Run(async()
    => await actionContext.Request.Content.ReadAsStringAsync()
        .ConfigureAwait(false)).Result;
    //rest of code omitted for brevity.
}

I'm trying to work out two things:

1.Will this code cause a deadlock?

2.Since the method cannot be marked async Task, should it be written like this instead?

var body = actionContext.Request.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult();
David Klempfner
  • 8,700
  • 20
  • 73
  • 153
  • 1
    Why would this cause a `deadlock`? A `deadlock` is, essentially, two threads waiting on each other to release a resource. There is no opportunity here to cause a `deadlock`. Now `ConfigureAwait(false)` in the OLD .net Framework MVC/WebApi was needed because IIS used main thread, and ONLY main thread to receiver requests. This is why await-async model was so important to relieve the main thread so it can take more incoming HTTP requests. under Kestrel and modern .net this is no longer an issue. – zaitsman Nov 07 '22 at 02:35
  • I thought there could be a deadlock due to the `.Result`. – David Klempfner Nov 07 '22 at 02:36
  • I am lost why you think there would be a deadlock? what else is waiting on this code? what is the code itself waiting on? (hint: it is waiting for request stream to be read by the underlying HTTP server). – zaitsman Nov 07 '22 at 02:37
  • Because of articles like https://medium.com/rubrikkgroup/understanding-async-avoiding-deadlocks-e41f8f2c6f5d. We are always warned against calling `Result` because of potential deadlocks. I don't think it will deadlock because of `ConfigureAwait(false)`, but I just wanted to confirm. – David Klempfner Nov 07 '22 at 02:40
  • Ok so again, this article clearly tells you that the problem exists when you have one main dispatching thread running your threadpool. `ConfigureAwait(false)` exists specifically to allow the threadpool to be unblocked if you dispatched work FROM threadpool thread TO threadpool thread (essentially in the situation where you made your thread wait on itself waiting to finish the work). TL:DR Use .net 6 and Kestrel and it will go away. – zaitsman Nov 07 '22 at 02:46
  • 1
    I would simply write: `var body = await actionContext.Request.Content.ReadAsStringAsync();` and then make the eventhandler `async`. Then the UI thread will stay responsive. – Poul Bak Nov 07 '22 at 03:47
  • @PoulBak but there is no UI thread because it's ASP.NET not WPF/WinForms. – David Klempfner Nov 07 '22 at 04:54
  • That's true, but the advice still stands. – Poul Bak Nov 07 '22 at 05:01
  • @zaitsman " because IIS used main thread, and ONLY main thread to receiver requests" - you are mixing up UI thread in WinForms/WPF and request thread in ASP.Net. There is no "main thread" in IIS. – Alexei Levenkov Nov 07 '22 at 07:56
  • @zaitsman it can deadlock because it will eventually run out of threadpool threads when their site will have some decent load... See José Ramírez answer for the exaplantion. – Alexei Levenkov Nov 07 '22 at 07:58
  • 2
    @zaitsman `ConfigureAwait(false) in the OLD .net Framework MVC/WebApi was needed because IIS used main thread, and ONLY main thread to receiver requests` - this is incorrect. ASP.NET has always supported parallel request processing, and has never had a single main thread. You may be thinking of Node.js, which does use a main thread model. – Stephen Cleary Nov 07 '22 at 08:43

2 Answers2

2

ConfigureAwait(false) is used to skip any synchronization context and go directly to the thread pool for the continuation work. So I am thinking you are on to something here for the cases where the synchronization context is a problem (single-threaded contexts), such as Windows Forms.

The problem, in essence, is that if you lock the synchronization context's only thread with a .Result or .Wait(), then there is no thread for any continuation work to be completed, effectively freezing your application. By ensuring a thread pool thread will complete the job using ConfigureAwait(false), then, at least in my head, all should be good as you are suspecting. I think this is a very nice find from you.

This however, as pointed out already in the given comments, is not your case for the particular code that you show. For starters, you are explicitly starting a new thread. That alone takes you away from the case you are fearing.

CLARIFICATION: A Task Is Not a Thread

As stated in the comment, I'll clarify. I simplified by saying "starting a new thread" in the last paragraph. I'll elaborate for the sake of the more enlightened.

Whenever you use Task.Run(), the newly created task will go sit in a thread pool task queue. There is the global queue, and there is one queue per thread pool thread. This is the fun part:

  • If the thread executing the Task.Run() statement is a thread pool thread, the new task will be placed last in that thread's task queue.
  • If the thread executing the Task.Run() statement is not a thread pool thread, the new task will be placed last in the global task queue.

At this point, since we are assuming we are not awaiting and instead we are putting the executing thread to sleep until the task is done, either using .Result or .Wait(), we enter the following situations:

  • A: The executing thread was a thread pool thread. This thread is now blocked and therefore 100% incapacitated to dequeue anything from its own task queue.
  • B: The executing thread was not a thread pool thread. The thread is now blocked, and the task went to the global queue.

If the hill-climbing algoritm is currently creating new threads, or the minimum number of threads has not yet been reached, new thread pool threads will be created. Each of the new threads will dequeue from the global task queue. If no tasks are there, they will proceed to steal work from individual threads' task queues.

This means that, by .Result'ing or .Wait()'ing from a thread has put us in the delicate position of having to wait for another thread pool thread to save us. This is ... brittle? What if the global queue is receiving tasks at a pace faster than the rate new threads are added to the thread pool? Then we have a frozen thread as if it had deadlocked.

This is why using .Result or .Wait() is generally discouraged.

Anyway, I simplified the way I did because in either case, you have to wait for a different thread to come to your rescue. By writing code like that you have guaranteed that the current thread will never be able to save itself. Hence, "new thread".

José Ramírez
  • 872
  • 5
  • 7
  • "you are explicitly starting a new thread" - this is not quite true. A task is not a thread. – Klaus Gütter Nov 07 '22 at 05:05
  • Agreed. Not really true. Task.Run() is creating the task and setting it in the thread's queue (if the executing thread is a thread pool thread), or the global queue. It is, however a thread pool task that will be picked up either by an existing thread or a new thread if the hill-climbing algorithm is currently creating new threads. – José Ramírez Nov 07 '22 at 05:23
  • That's quite philosophical observation "Then we have a frozen thread as if it had deadlocked." - indeed it is the definition of [deadlock](https://en.wikipedia.org/wiki/Deadlock) when none of the threads can continue because all of them will be waiting till at least one is freed up... (Note that OP does not create new threads, `Task.Run` picks threads from the same thread pool) – Alexei Levenkov Nov 07 '22 at 07:55
  • Can you clarify "there is one queue per thread pool thread"? do you mean "there is one queue per **CPU** thread pool thread"? – Paulo Morgado Nov 07 '22 at 08:34
2

Will this code cause a deadlock?

string body = Task.Run(async() =>
{
    return await actionContext.Request.Content.ReadAsStringAsync()
        .ConfigureAwait(false);
}).Result;

No. The ReadAsStringAsync task is invoked on the ThreadPool, and the ThreadPool has no synchronization context. So the await will find no synchronization context to capture. For this reason the .ConfigureAwait(false) has no effect. You can omit it, or set the continueOnCapturedContext to true, and the result will be the same: no deadlock.

Should it be written like this instead?

string body = actionContext.Request.Content.ReadAsStringAsync()
    .ConfigureAwait(false).GetAwaiter().GetResult();

This code will cause no deadlock either. The reason is because the ReadAsStringAsync method does not capture the synchronization context internally. Any await in its internal implementation is configured with .ConfigureAwait(false). This is something beyond your control. It's just how this method is implemented. The .ConfigureAwait(false) that you have inserted before the .GetAwaiter().GetResult() has no effect. The ConfigureAwait method affects the behavior of await, when a synchronization context is present. In your case there is no await, so whether a synchronization context is present or not is irrelevant. The .GetAwaiter().GetResult() is not await. So it doesn't matter if you set the continueOnCapturedContext to false or true, or omit the ConfigureAwait altogether. The result will be the same: no deadlock.

Since your code is part of an ASP.NET application, none of these two approaches is recommended. Both are blocking needlessly a ThreadPool thread while the ReadAsStringAsync operation is in-flight.

Update: As Alexei Levenkov pointed out in the comments below, blocking on async code with .Wait() or .Result or .GetAwaiter().GetResult() in ASP.NET applications carries the risk of a complete and total deadlock of the whole site. This can happen in case the ThreadPool availability has been exhausted. The ThreadPool can create quite a lot of threads, precisely 32,767 threads in my Windows 10 / 64-bit machine running .NET 6 (as I found out by calling the ThreadPool.GetMaxThreads method), so exhausting it is not an easy feat. But if your site is busy and you are blocking on async code a lot, it's not impossible. If this happens then the completing asynchronous operations will find no available threads to schedule their continuations. The .Wait() is blocking the current thread on a ManualResetEventSlim, that waits for a signal from another thread. The signal will never come, and so the .Wait() will block forever. Then the whole site will come to a complete halt. No more requests are going to be served, until the process is recycled. Here is a related article by Stephen Toub: Should I expose synchronous wrappers for asynchronous methods?

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • Note that your answer is wrong for ASP.Net as it will deadlock when you run out of threadpool threads (since each `.Result` blocks one threadpool thread). It will only happen when your site is in use - which is not the case for most people :). – Alexei Levenkov Nov 07 '22 at 07:45
  • @AlexeiLevenkov are you referring to the first approach (with `Task.Run`) or the second? By *"when you run out of threadpool threads"* do you mean when the [`ThreadPool.GetAvailableThreads`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.threadpool.getavailablethreads) will be reduced to zero? – Theodor Zoulias Nov 07 '22 at 08:28
  • @AlexeiLevenko btw the `ThreadPool` has 32,767 available `workerThreads` in my PC. That's a lot of threads. If an application manages to exhaust the `ThreadPool` availability, without other resources having already run out, then I would argue that the situation would be described better as "scalability limit" than "deadlock". It's a well known fact that async-enabled applications are more scalable than applications that block threads. – Theodor Zoulias Nov 10 '22 at 10:51
  • 1
    Either approach - `.Result` blocks one thread in the threadpool, so as long as you have enough incoming requests and wait time long enough, you'll run out of threadpool threads. Maybe latest limits are better... I personally wrote code that way with a comment along the lines of "this is rare fast call with result cached so `.Result` is ok" and about a year later one day it was not a fast call, not cached and traffic happened to be shaped just right. Indeed, looking at those thousands of stacks in the dump was educational. – Alexei Levenkov Nov 10 '22 at 17:12
  • @AlexeiLevenko interesting. What was the outcome? Did the whole site came to a permanent and complete halt? Or just everything was happening very slowly? – Theodor Zoulias Nov 10 '22 at 17:26
  • 1
    Stopped handled requests completely and as far as I remember stuck in that state forever, even when we cut out traffic. One day I will setup an example and try on current versions... but for practical purposes I consider that synchronous waiting for the result of a real potentially long async operation will deadlock whole process in the right conditions. – Alexei Levenkov Nov 10 '22 at 17:40
  • @AlexeiLevenkov so the `string body = httpClient.GetStringAsync(url).Result` might kill the site with deadlock, while the `string body = webClient.DownloadString(url)` doesn't have the same risk? Do you believe that the `GetStringAsync` needs at some point a `ThreadPool` thread to complete, while the synchronous `DownloadString` doesn't depend on the `ThreadPool`? – Theodor Zoulias Nov 10 '22 at 18:06
  • 1
    Synchronous version *may* actually work out better in that particular as there likely no code that need to pick a thread from a threadpool to handle "run some code on a random thread before returning results" (the code between await and final returns inside `GetStringAsync` needs to run somewhere). I expect synchronous versions to use some other mechanism than .Net threadpool to get notified about data availability and likely have built-in timeouts (unlike default `.Result`). So my rating: async is better than sync, sync is better or the same as `.Result` for scalability/chances of deadlock. – Alexei Levenkov Nov 10 '22 at 18:36
  • @AlexeiLevenkov thanks. I think that this is a very interesting topic. I might post a new question about it. – Theodor Zoulias Nov 10 '22 at 18:41
  • Ping me if you ask - I agree that this whole topic is interesting and I'd happy to learn more. For sync/async versions it may be helpful to write pseudocode for both like: {PrepareRequest, CallNativeAsyncMethod with callback/notification, ProcessRequest(will run on ThreadPool), ReturnResult to caller (send to the other thread sitting in `.Result`)} vs { PrepareRequest, CallNativeMethod and wait, ProcessRequest (on current thread), ReturnResult (on current thread)}. `ProcessResult` in async case have to have managed thread to run on. – Alexei Levenkov Nov 10 '22 at 19:04
  • 1
    @AlexeiLevenkov I just read an old article by Stephen Toub, that confirms the risk of deadlock in a `ThreadPool`-starvation scenario: [Should I expose synchronous wrappers for asynchronous methods?](https://devblogs.microsoft.com/pfxteam/should-i-expose-synchronous-wrappers-for-asynchronous-methods/) – Theodor Zoulias Nov 17 '22 at 07:56