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

Potential Deadlock #91

Closed
anirudhsanthiar opened this Issue Feb 9, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@anirudhsanthiar

anirudhsanthiar commented Feb 9, 2016

The Connect method in TcpTransport.cs could deadlock. The method is:

 public void Connect(Connection connection, Address address, bool noVerification)
 {
    this.connection = connection;
    var factory = new ConnectionFactory();
    if (noVerification)
    {
        factory.SSL.RemoteCertificateValidationCallback = noneCertValidator;
    }

    this.ConnectAsync(address, factory).GetAwaiter().GetResult();
}

A GUI client (such as a Windows Forms app) or an MVC client will deadlock if it calls the method as follows:

const string Endpoint = "amqp://guest:guest@localhost:5765";           
Address address = new Address(Endpoint);
var connection = new Connection(address);

(The constructor for the Connection object eventually calls the Connect method above.)

Note that the connect method blocks on a Task object because of the call to GetResult:
this.ConnectAsync(address, factory).GetAwaiter().GetResult();
Blocking on Task objects (Via Task.Wait() or Task.Result or Task.GetAwaiter().GetResult())
is dangerous and could result in deadlocks.
(See, for example http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html)

@xinchen10

This comment has been minimized.

Member

xinchen10 commented Feb 9, 2016

@anirudhsanthiar you are right. There are also other methods (e.g. SenderLink.Send, ReceiverLink.Recieve) that are blocking on an underlying task. If they are called from a UI thread, the same issue will happen. In such cases, the async methods should be called. For example,

    Connection connection = await Connection.Factory.CreateAsync(address);
    ....
    await connection.CloseAsync();

I think we could either improve the documentation to make it obvious or remove the blocking APIs on certain platforms.

@anirudhsanthiar

This comment has been minimized.

anirudhsanthiar commented Feb 10, 2016

Yes, that would be great. Another option is to configure the await statements in ampqnetlite to not capture context via ConfigureAwait(false) [see, for example, the section under "avoiding context" in 1]

@xinchen10

This comment has been minimized.

Member

xinchen10 commented May 4, 2016

The documentation for connection constructors has been updated. The code is also updated to not capture context.

@xinchen10 xinchen10 closed this May 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment