-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625
Changes from 4 commits
3c59110
cc922ae
be88299
6b028a7
949acf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1275,6 +1275,20 @@ private async ValueTask SendDataAsync(ReadOnlyMemory<byte> buffer, CancellationT | |
await _connection.SendStreamDataAsync(StreamId, current, flush, _requestBodyCancellationSource.Token).ConfigureAwait(false); | ||
} | ||
} | ||
catch (OperationCanceledException e) when (e.CancellationToken == _requestBodyCancellationSource.Token) | ||
{ | ||
if (_resetException is Exception resetException) | ||
{ | ||
if (_canRetry) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to take the lock around checking _resetException and _canRetry. Otherwise we may see an inconsistent view of these two values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I overlooked it. Fixed. |
||
{ | ||
ThrowRetry(SR.net_http_request_aborted, resetException); | ||
} | ||
|
||
ThrowRequestAborted(resetException); | ||
} | ||
|
||
throw; | ||
} | ||
finally | ||
{ | ||
linkedRegistration.Dispose(); | ||
|
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.
Should we check and throw the exception the same way as
CheckResponseBodyState
does in:runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Lines 923 to 931 in 56778f0
ProcessGoAwayFrame
sets_canRetry
totrue
AFAICT and we're not handling it here.Could we just directly call
CheckResponseBodyState
in the exception handling block?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.
_canRetry
check has been added, but we cannot callCheckResponseBodyState
here because it asserts for a lock like thisDebug.Assert(Monitor.IsEntered(SyncObject));
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.
Should we just take the lock and call it, then?
CheckResponseBodyState also checks for
_responseProtocolState == ResponseProtocolState.Aborted
; should we be checking that here too?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 think we should not check
_responseProtocolState
here becauseSendDataAsync
is called to send request data, so I believe it should control request-related logic only. That's another reason why I think we shouldn't callCheckResponseBodyState
here.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.
@geoffkizer Could you please check out my reply above?
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.
Ok, your comment sounds right to me.