Skip to content

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 2, 2021
Merged

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

merged 5 commits into from
Jul 2, 2021

Conversation

@alnikola
Copy link
Contributor

@alnikola alnikola commented Jun 23, 2021

If server sends GO_AWAY to client, Http2Connection handles it and sets a correct Http2ConnectionException to _resetException field followed by resetting all active Http2Streams. Each of these streams is expected to rethrow that _resetException to communicate the original protocol error to the application code. However, the method Http2Stream.SendDataAsync currently doesn't take into account that field, thus when it gets cancelled as part of a stream reset it just throws OperationCanceledException which doesn't contain any details. This PR fixes that and makes Http2Stream.SendDataAsync throw the original Http2ConnectionException wrapped by IOException.

Fixes #42472

@alnikola alnikola requested a review from a team June 23, 2021 18:01
@ghost ghost added the area-System.Net.Http label Jun 23, 2021
@ghost
Copy link

ghost commented Jun 23, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

If server sends GO_AWAY to client, Http2Connection handles it and sets a correct Http2ConnectionException to _resetException field followed by resetting all active Http2Streams. Each of these streams is expected to rethrow that _resetException to communicate the original protocol error to the application code. However, the method Http2Stream.SendDataAsync currently doesn't take into account that field, thus when it gets cancelled as part of a stream reset it just throws OperationCanceledException which doesn't contain any details. This PR fixes that and makes Http2Stream.SendDataAsync throw the original Http2ConnectionException wrapped by IOException.

Fixes #42472

Author: alnikola
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@alnikola alnikola added this to the 6.0.0 milestone Jun 23, 2021
@karelz karelz mentioned this pull request Jun 24, 2021
Closed
@karelz karelz requested review from ManickaP and geoffkizer June 24, 2021 15:49
ManickaP
ManickaP reviewed Jun 25, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, just a question about the exception handling.
Thanks!

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs Outdated
}
catch (Exception)
{
if (_resetException is Exception resetException)
Copy link
Member

@ManickaP ManickaP Jun 25, 2021

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

if (_resetException is Exception resetException)
{
if (_canRetry)
{
ThrowRetry(SR.net_http_request_aborted, resetException);
}
ThrowRequestAborted(resetException);
}

ProcessGoAwayFrame sets _canRetry to true AFAICT and we're not handling it here.

Could we just directly call CheckResponseBodyState in the exception handling block?

Copy link
Contributor Author

@alnikola alnikola Jun 28, 2021

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 call CheckResponseBodyState here because it asserts for a lock like this
Debug.Assert(Monitor.IsEntered(SyncObject));

Copy link
Contributor

@geoffkizer geoffkizer Jun 28, 2021

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?

Copy link
Contributor Author

@alnikola alnikola Jun 29, 2021 •
edited
Loading

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 because SendDataAsync 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 call CheckResponseBodyState here.

Copy link
Contributor Author

@alnikola alnikola Jul 1, 2021

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?

Copy link
Contributor

@geoffkizer geoffkizer Jul 1, 2021

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.

@alnikola
Copy link
Contributor Author

alnikola commented Jun 28, 2021

@geoffkizer @ManickaP All comments are addressed. Please, review.

ManickaP
ManickaP approved these changes Jun 28, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

geoffkizer
geoffkizer reviewed Jul 1, 2021
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs Outdated
{
if (_resetException is Exception resetException)
{
if (_canRetry)
Copy link
Contributor

@geoffkizer geoffkizer Jul 1, 2021

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@alnikola alnikola Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I overlooked it. Fixed.

geoffkizer
geoffkizer suggested changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@geoffkizer geoffkizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the locking issue I just mentioned above, LGTM.

@alnikola alnikola merged commit 4c92aef into dotnet:main Jul 2, 2021
@alnikola alnikola deleted the alnikola/42472-goaway-throw-ioexception branch July 2, 2021 09:00
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 2, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_hot_reload_debug
946c391 
* origin/main: (27 commits)
  [mono][llvm] Only emit 'LLVM failed' messages on verbosity > 0. (dotnet#55060)
  Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY (dotnet#54625)
  [main] Update dependencies from dnceng/internal/dotnet-optimization dotnet/arcade dotnet/xharness dotnet/hotreload-utils (dotnet#55007)
  disable a failing test. (dotnet#55063)
  [mono][wasm] Disable some tests which crash on AOT. (dotnet#55054)
  Fix fix_allocation_context for regions (dotnet#54931)
  Delete stale references to System.IO.FileSystem.Primitives (dotnet#55041)
  Add binplaced analyzers to ASP.NET transport package (dotnet#55042)
  [mono] Enable many HardwareIntrinsic tests on wasm
  Delete `compQuirkForPPP`. (dotnet#55050)
  [Mono] Condition Workload AOT import to be osx only (dotnet#55040)
  package native quic library (dotnet#54992)
  Make GlobalizationMode code consistent (dotnet#55039)
  Expand PerfMap format to support metadata for symbol indexation (dotnet#53792)
  [debugger]Componentize debugger (dotnet#54887)
  [Mono] Include loaded interpreter methods as EventPipe session rundown method events. (dotnet#54953)
  Delete stale ActiveIssue from HttpHeadersTest (dotnet#55027)
  Poison address-exposed user variables in debug (dotnet#54685)
  Recategorize emsdk dependency (dotnet#55028)
  Remove the the wasm AOT specific test project exclusions (dotnet#54988)
  ...
@geoffkizer geoffkizer mentioned this pull request Jul 10, 2021
Merged
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ManickaP ManickaP ManickaP approved these changes

+1 more reviewer

@geoffkizer geoffkizer geoffkizer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@geoffkizer geoffkizer

Labels

area-System.Net.Http

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

HTTP/2 stress test TaskCanceledException when client hasn't cancelled

3 participants

@alnikola @ManickaP @geoffkizer