Task.Result and Task.Wait design flaw leads to deadlock in high loaded applications

[AqlaSolutions]

Run this application:

https://gist.github.com/AqlaSolutions/f8c993bfad32dc9be11703fea4e01422

It simulates network engine receiving requests and calling synchronously IConnection.OnRequestReceived. This method calls async method and then synchronously processes the result retrieved with Task.Result. The async method may take up to 700 ms.

During run the simulation will decrease time between requests until deadlock occurs - after that point currently awaiting requests will never be completed.

This example proves that absolutely any call to Task.Result or Task.Wait (before task completion) from ThreadPool thread will work for some time but will eventually deadlock your application in truly high load scenario. Using these methods from ThreadPool thread for waiting is always a mistake and should be disallowed (but I understand that it will not happen for compatibility reasons).

What do you think? May be some workarounds inside .NET TP? Prioritize continuations over new tasks?

[benaadams]

Task.Result and Task.Wait block the current thread you should use await. (Though they only block if not already completed)

The workaround if you are blocking threadpool threads is to set threadpool minthreads to a high number; though that isn't very scalable.

The main workaround is to not use blocking methods in threadpool threads; and there are alternatives.

[AqlaSolutions]

@beannaich,

1. Assume I can't use await, because external network engine provides synchronous interface and I need to consider async call result before returning.
2. Task.Result is just not recommended. When you put it there it actually looks like it works ok and I think many people do that.
3. Setting TP max threads to high number will not fix the possibility of deadlock but may affect your app performance in a bad way.

[benaadams]

You are basically hobbling the threadpool with

ThreadPool.SetMinThreads(2, 200);
ThreadPool.SetMaxThreads(10, 200);

try

ThreadPool.SetMinThreads(200, 200);
ThreadPool.SetMaxThreads(1000, 200);

and you probably won't have any problems; though its not a good solution

[AqlaSolutions]

@benaadams, I set TP to low threads number to show the problem. It will also happen on 200 and 1000 threads but it will just take more time and will require modifying the simulation code for more requests (server may have 50000 ccu, what will you do?). In a real app you will never want to set max threads to such big numbers because it will produce too much threads and make your app very very slow because of context switching thing. Also don't forget that each thread takes 1 MB RAM.

to not use blocking methods in threadpool threads

I use NHibernate everywhere in our project and it's blocking. It's impossible to not use.

[HaloFour]

https://msdn.microsoft.com/en-us/magazine/jj991977.aspx

In short, don't mix async and sync. "Async all the way down." If you can't fit in that model you're kind of stuck.

As for the potential of changing this, those would all be BCL concerns, not language concerns. C# doesn't know nor care about ThreadPool or Task. Check out the https://github.com/dotnet/corefx repo.

[benaadams]

I use NHibernate everywhere in our project and it's blocking. It's impossible to not use.

Raise an issue with NHibernate? A blocking call will block a thread and as you've pointed out the issues with that.

[AqlaSolutions]

@HaloFour, when "you can't fit" almost everyone suggests to "just use .Result here". They all know about "not mixing" but .Result looks like exactly for mixing. This deadlock is not obvious and someone may never encounter it during testing.

NHibernate authors already stated that they are not going to support async and that it shouldn't affect performance when compared to app logic. This is an example how "good" people usually understand TP. Do you still think they won't use .Result in TP threads?

Anyway NHibernate is a very big project and I don't believe that it can be easily migrated to async.

So for my project I will create async wrappers for the most blocking thing in NHibernate - getting connection from pool - which will move blocking calls to a separate custom TP and free the main TP for continuations.

I agree, this issue should belong to corefx.

[HaloFour]

If NH thinks that DB I/O time is best spent blocking threads then you should seriously find another solution as they are clearly criminally incompetant. If they're the ones recommending to "just use .Result" that further validates my opinion. MS docs very clearly state to not do this, as does every article they've written on the subject.

[benaadams]

If you have to shoot yourself in the foot use .GetAwaiter().GetResult() rather than .Result, it will give a better exception if something went wrong.

server may have 50000 ccu, what will you do?

You will have an issue scaling to large numbers of concurrent requests as blocking methods require threads to block - whether you use the threadpool or not; blocking in the threadpool just has other bad side-effects as its a shared resource.

[CyrusNajmabadi]

Roslyn does not appear to be the right repo for this issue.

[terrajobst]

@AqlaSolutions

Unless you think the problem is in the code generated by the Roslyn compiler, you should close this issue and file a new one against the Task APIs which are part of the http://github.com/dotnet/corefx repository.

[AqlaSolutions]

https://github.com/dotnet/corefx/issues/11463

[terrajobst]

Thanks!