-
Notifications
You must be signed in to change notification settings - Fork 49
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
[BUG] Deadlock prone code in HttpConnection.cs #689
Comments
Hey @DavidPoulsenHGS, Thanks for reporting this, this is definitely something we'd want to address. I think the currently in-dev next major version would provide a good opportunity to handle this and add the additional target framework for .NET 6.0+. Is this something you'd be potentially interested in looking into and contributing? No pressure if not! |
Perhaps, depending on the complexity. I'd like to give it a shot, at least. How would I go about doing this? Do I just fork the 1.x branch and submit a pull request? |
@DavidPoulsenHGS All changes should generally go into the Even partial solutions can be helpful, so if you do feel stuck you can always raise a draft pull request and I can help give feedback or try to guide things in the right direction. |
… Changed synchronous methods in HttpConnection.cs to use Task.Run/await to avoid deadlocks in accordance with: https://learn.microsoft.com/en-us/archive/blogs/jpsanders/asp-net-do-not-use-task-result-in-main-context
I have made a pull request for my fix now: #737 The unit tests fail on my machine on a clean checkout (without my fix), so it's quite difficult for me to test that my change works. It is a very simple fix - perhaps someone else can run the unit tests on it? |
What is the bug?
The HttpConnection class contains the following code in the Request method:
responseMessage = client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead).GetAwaiter().GetResult();
and a little further into the same method:
responseStream = responseMessage.Content.ReadAsStreamAsync().GetAwaiter().GetResult();
These two lines of code are prone to deadlocks due to calling GetAwaiter().GetResult(). This link explains why.
A surprisingly similar issue has been discussed to great length in an ElasticSearch.Net issue as well.
How can one reproduce the bug?
It's difficult to reproduce. Essentially requires continuously calling the Request method from multiple threads to provoke a deadlock. It can be quite common under load in a production environment, however.
What is the expected behavior?
The code should be free of potential deadlocks. I suggest changing the implementation to use the synchronous "Send" method of the System.Net.HttpClient instead.
The text was updated successfully, but these errors were encountered: