[BUG] Deadlock prone code in HttpConnection.cs #689#737
Conversation
… 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
DavidPoulsenHGS
requested review from
MaxKsyunz,
VachaShah,
Xtansia,
Yury-Fridlyand,
anirudha,
dblock and
joshuali925
as code owners
| { | ||
| receive = DiagnosticSource.Diagnose(DiagnosticSources.HttpConnection.ReceiveBody, requestData, statusCode); | ||
| responseStream = responseMessage.Content.ReadAsStreamAsync().GetAwaiter().GetResult(); | ||
| var task = Task.Run(() => responseMessage.Content.ReadAsStreamAsync()); |
There was a problem hiding this comment.
have you considered just changing it to responseMessage.Content.ReadAsStreamAsync().ConfigureAwait(false)?
| threadPoolStats = ThreadPoolStats.GetStats(); | ||
|
|
||
| responseMessage = client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead).GetAwaiter().GetResult(); | ||
| var task = Task.Run(() => client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead)); |
There was a problem hiding this comment.
have you looked at using client.SendAsync(requestMessage, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait(false)?
There was a problem hiding this comment.
@joefeser .ConfigureAwait(false) is not needed inside a Task.Run since there is no synchronization context to capture.
|
Using .GetAwaiter().GetResult() in a library is considered a massive problem since it does open the door to deadlocks. As a last resort, Task.Run(...).Wait() is definitely better than the current .GetAwaiter().GetResult(), but the more appropriate option is to remove the synchronous interface altogether if it's just wrapping an async call in a synchronous wait. At that point, the user of the library can do that themselves using their own judgment on whether it's necessary and with full knowledge of what is happening. |
3 participants
Description
Fixes deadlock prone code in HttpConnection.cs. Changed synchronous methods in HttpConnection.cs to use Task.Run/Task.Wait 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
Issues Resolved
#689
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.