-
Notifications
You must be signed in to change notification settings - Fork 135
Fixed deadlock when calling SendQueryAsync synchronously #332 #337
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
Conversation
@@ -87,7 +87,8 @@ public async Task<GraphQLResponse<TResponse>> SendQueryAsync<TResponse>(GraphQLR | |||
Options.EndPoint.HasWebSocketScheme()) | |||
return await GraphQlHttpWebSocket.SendRequest<TResponse>(request, cancellationToken); | |||
|
|||
return await SendHttpRequestAsync<TResponse>(request, cancellationToken); | |||
return await SendHttpRequestAsync<TResponse>(request, cancellationToken) | |||
.ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only there and not two lines above also ?
@@ -143,7 +144,8 @@ private async Task<GraphQLHttpResponse<TResponse>> SendHttpRequestAsync<TRespons | |||
var preprocessedRequest = await Options.PreprocessRequest(request, this); | |||
|
|||
using var httpRequestMessage = preprocessedRequest.ToHttpRequestMessage(Options, JsonSerializer); | |||
using var httpResponseMessage = await HttpClient.SendAsync(httpRequestMessage, HttpCompletionOption.ResponseHeadersRead, cancellationToken); | |||
using var httpResponseMessage = await HttpClient.SendAsync(httpRequestMessage, HttpCompletionOption.ResponseHeadersRead, cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only there and not two lines below also ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deadlock happens exactly because of HttpClient.SendAsync
has no ConfigureAwait(false)
. To resolve issue #332 it's enougth to call ConfigureAwait(false)
just here. But idealy whole project should be reviewed and each await
should be configured. First, it will prevent from potential deadlocks, and second, it may have positive impact on performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deadlock happens exactly because of
HttpClient.SendAsync
has noConfigureAwait(false)
Could you please write a test to verify this assertion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you meen a unit test or just simple test application to be sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit test to be able to run on CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may call it unit or integration, whatever. The key part is to verify behavior on CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there's somthing to test. Also I'm not sure it's posible to test ConfigureAwait
or dedlock prevention. So I can't write it.
The following changes fixes deadlock problem from issue #332.
ConfigureAwait(false)
onSendHttpRequestAsync
andHttpClient.SendAsync
.