-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mix of Async and Sync methods may result in deadlocks #237
Comments
I noticed this yesterday in my testing so I can confirm that this issue still occurs. |
We to have an issue where we think our application hits a deadlock. |
We've had deadlocks and slowdowns due to SendAsync continuations as well. The fix we put in place is to always call As I discussed with @xinchen10 previously I believe this should be the default behaviour of |
Things work fine if you use SendAsync() with CloseAsync()...or Send() with Close(). So if that's a change you can make, that should work. |
"It is worth adding that similar mixes work with no issues on standard .NET APIs (you may use any version of the methods – sync or async and it just works)." @PiotrSpikowski : What do you mean by standard .NET APIs here.? |
Taking into consideration that I can't mix asynchronous and synchronous methods I have two questions:
|
@nicodeslandes @xinchen10 It’s been a while but the issue still exists. You can easily end up in a deadlock or block/pause a message processing pump. I am now thinking about a different way allowing us to resolve this issue. A small recap.. you can have a deadlock e.g. by sending and then by trying to close a link: await sender.SendAsync(message).ConfigureAwait(false);
sender.Close(); // You will have a deadlock here You can also block/pause a message pump depending on what your code (continuation) is doing: await sender.SendAsync(message).ConfigureAwait(false);
Thread.Sleep(60 * 1000); // By performing any blocking operations you will also block a message pump The problem is related into a way in which By default task’s continuations run synchronously. This also happens when the I am now thinking about one possible correction for all the deadlocks. We could allow continuations to run asynchronously. It can be realised with help of the Without the above fix one may still be forced to introduce multiple ugly workarounds. For example, after a call into every method involving a await sender.SendAsync(message).ConfigureAwait(false);
await Task.Yield(); // For continuation to run asynchronously.
sender.Close(); // It will NOT deadlock here. We should be on a different thread. |
That would certainly help first time users not falling into this trap. It might be a good idea to optionally allow synchronous continuation, for users worried about the performance implications, and who know for sure they will not run into deadlocks. |
This is by design, not an issue. There is no reason to call the sync methods after await when the async version exists. I agree that it might be possible that the user makes a call to a blocking API in a different library. But in that case the code will never work as expected so the problem can be fixed at development time. |
@xinchen10 I fully agree here with @nicodeslandes. Looking at this problem from a library “consumer” perspective I would expect it to work by default. If mixing of Sync and Async methods is not allowed by design then we could have them separated (e.g. we could have separate Link classes for sync and async operations or we could have separate public contracts (interfaces) for sync/async so the intended usage is clear from the start). Otherwise it might be hard to convince a consumer that a tricky to detect deadlock is by design. As the library is using the I’m pretty sure that the problem is common for different solutions having something like a message pump. I have quickly checked the Microsoft.Azure.Amqp for comparison and its |
If you are interested you can check out a similar discussion in a bigger scope here: dotnet/runtime#14882 The question is really about the default behavior. For this library we will use whatever the default parameters are from the framework (which means synchronous continuation). We could add extension methods or a config knob for the task creation options for asynchronous continuation but that won't help much if a user is not aware of the threading issue at all. We will leave these to extension and higher-level libraries. @PiotrSpikowski yes the Microsoft.Azure.Amqp is doing the workaround many other libraries are doing by scheduling all callbacks on the thread pool. Separating the methods is a good idea but we cannot prevent someone from calling task.Result or task.Wait(). |
We've just fallen foul of this - any plans to fix this issue? |
The library is exposing methods in both versions: async and sync. For example: we have
SendAsync
andSend
,CloseAsync
andClose
.The mix of Async and Sync method calls might not be recommended. When one is using Async methods then he should probably prefer Async versions for all of the operations. There are however cases in which the mix seems to be reasonable and can be done as both APIs are given by the library. The additional reasoning can also be that not everything is async those days in .NET. Consider for example the
IDisposable
interface and theusing
statements.Skipping the considerations when async and sync version should be used, how async and sync items should be disposed and similar aspects (which are for a separate post) it is fairly easy to write something like this:
The above will open a connection synchronously. It will send a message using the async version and then it will try to close the objects using synchronous versions again.
The code will open the connection, send a message but it will deadlock when trying to close the objects. It is worth adding that similar mixes work with no issues on standard .NET APIs (you may use any version of the methods – sync or async and it just works).
In the above sample if we do SendAsync then the callback (task completion) is run by the message pump. It is run when a Disposition is received. In a short, Disposition calls the SetResult on the TaskCompletionSource. SetResult blocks running the continuation so the main thread (message pump) cannot move and continue. The continuation itself tries to close the objects. Unfortunately close waits for an event (Detach) which can only be set by a message pump. The message pump is blocked waiting. We have a deadlock as the TaskCompletionSource.SetResult(…) invokes the code awaiting the task and it does it on the same thread as the message pump.
The text was updated successfully, but these errors were encountered: