Mix of Async and Sync methods may result in deadlocks

[PiotrSpikowski]

The library is exposing methods in both versions: async and sync. For example: we have SendAsync and Send, CloseAsync and Close.

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 the using 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:

Connection connection = new Connection(new Address("amqp://localhost:5672"), null, new Open
{
   // Some open params here ...
}, (_, __) => { });
Session session = new Session(connection);

Message message = new Message("Hello AMQP") { Properties = new Properties { To = "queue" } };

SenderLink sender = new SenderLink(session, "sender123", new Attach
{
   Role = false,
   SndSettleMode = SenderSettleMode.Unsettled,
   Target = new Target { Address = "queue" },
   // Other Attach params here ...
}, null);

await sender.SendAsync(message);

sender.Close(); // It will deadlock here
session.Close();
connection.Close();

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.

[spankr]

I noticed this yesterday in my testing so I can confirm that this issue still occurs.

[CodeGlitcher]

We to have an issue where we think our application hits a deadlock.
Have you found a workaround?

[nicodeslandes]

We've had deadlocks and slowdowns due to SendAsync continuations as well. The fix we put in place is to always call await Task.Yield() after calling SendAsync().

As I discussed with @xinchen10 previously I believe this should be the default behaviour of SendAsync(), as it's causing too many issues to too many people. You should have to opt out of it if you are concerned with the performance penalties it brings (which are benign for most users I would think).

[spankr]

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.

[ashish2rathi]

"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.?

[jan-maria]

Taking into consideration that I can't mix asynchronous and synchronous methods I have two questions:

1. ConnectionFactory lacks of synchronous Create() method - what should I do instead if I need an access to TCP settings and AMQ settings (available during instantiation of ConnectionFactory)?
2. I can use asynchronous methods all way down, but how do I asynchronously close connection from within synchronous Dispose() method of a class which is implementing IDisposable? Or maybe explicit calling of Close() method on IAmqpObject is just optional (wouldn't it leak handlers/memory?)?

[PiotrSpikowski]

@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 TaskCompletionSource is used (or in fact into a way in which all task continuations are run by default).

By default task’s continuations run synchronously. This also happens when the SetResult method is called on the TaskCompletionSource. In such a case an async continuation is invoked synchronously so in our scenario a continuation may run on the same thread as the message processing pump. When a message pump is blocked but we run something else needing this pump to continue (as the sync Close method) we will have deadlock.

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 TaskCreationOptions.RunContinuationsAsynchronously option (available from .NET 4.6). This option can also be passed into the ctor of every TaskCompletionSource e.g. new TaskCompletionSource(TaskContinuationOptions.RunContinuationsAsynchronously). All the places where TaskCompletionSources are used would have to be changed.

Without the above fix one may still be forced to introduce multiple ugly workarounds. For example, after a call into every method involving a TaskCompletionSource one would have to “manually” force continuation to run asynchronously e.g. with help of the Task.Yield(). So in our sample:

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.

[nicodeslandes]

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.
As I said above, I do believe asynchronous continuations (or possibly calls to Task.Yield()) should indeed be the default ("pit of success" and all that... 🙂).

[xinchen10]

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.
The issue was brought up with task usage but that's not the only way for the deadlock. The library also has callback based APIs and it will happen if same thing was done in the callback. Having a workaround for task based API is not a full solution.

[PiotrSpikowski]

@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 TaskCompletionSource setting the RunContinuationsAsynchronously option may help in multiple places. I don’t know about callbacks used by the lib but maybe there is something “challenging” in other places too.

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 SendingAmqpLink. I had no time to look at the internals but definitely my continuation runs asynchronously after a call to await link.SendMessageAsync(message). I guess an arrival of a disposition completes a task asynchronously e.g. using TaskContinuationOptions.RunContinuationsAsynchronously or maybe directly scheduling via the ThreadPool (depending on the library implementation)

[xinchen10]

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().

[KwalityKoder]

We've just fallen foul of this - any plans to fix this issue?