Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

alnikola · 2021-06-23T18:01:40Z

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

ghost · 2021-06-23T18:01:45Z

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:	-

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs

ManickaP

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

ManickaP · 2021-06-25T04:28:35Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs

@@ -1275,6 +1275,15 @@ private async ValueTask SendDataAsync(ReadOnlyMemory<byte> buffer, CancellationT
                        await _connection.SendStreamDataAsync(StreamId, current, flush, _requestBodyCancellationSource.Token).ConfigureAwait(false);
                    }
                }
+                catch (Exception)
+                {
+                    if (_resetException is Exception resetException)


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?

_canRetry check has been added, but we cannot call CheckResponseBodyState here because it asserts for a lock like this
Debug.Assert(Monitor.IsEntered(SyncObject));

Should we just take the lock and call it, then?

CheckResponseBodyState also checks for _responseProtocolState == ResponseProtocolState.Aborted; should we be checking that here too?

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.

@geoffkizer Could you please check out my reply above?

Ok, your comment sounds right to me.

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

alnikola · 2021-06-28T10:09:49Z

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

ManickaP

LGTM, thanks

geoffkizer · 2021-07-01T15:15:48Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs

+                {
+                    if (_resetException is Exception resetException)
+                    {
+                        if (_canRetry)


I think we need to take the lock around checking _resetException and _canRetry. Otherwise we may see an inconsistent view of these two values.

Good point. I overlooked it. Fixed.

geoffkizer

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

* 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) ...

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY

3c59110

alnikola requested a review from a team June 23, 2021 18:01

dotnet-issue-labeler bot added the area-System.Net.Http label Jun 23, 2021

alnikola added this to the 6.0.0 milestone Jun 23, 2021

karelz mentioned this pull request Jun 24, 2021

Connect_DualMode_DnsConnect_RetrievedEndPoints_Success - Assert ::1 #54677

Closed

karelz requested review from ManickaP and geoffkizer June 24, 2021 15:49

geoffkizer reviewed Jun 24, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs Outdated Show resolved Hide resolved

ManickaP reviewed Jun 25, 2021

View reviewed changes

alnikola added 2 commits June 28, 2021 12:05

Catching only OperationCanceledException with the specific token

cc922ae

_canRetry is checked

be88299

ManickaP approved these changes Jun 28, 2021

View reviewed changes

Fix test

6b028a7

karelz assigned geoffkizer Jun 29, 2021

geoffkizer reviewed Jul 1, 2021

View reviewed changes

geoffkizer suggested changes Jul 1, 2021

View reviewed changes

Add lock around _resetException and _canRetry

949acf6

geoffkizer approved these changes Jul 1, 2021

View reviewed changes

alnikola merged commit 4c92aef into dotnet:main Jul 2, 2021

alnikola deleted the alnikola/42472-goaway-throw-ioexception branch July 2, 2021 09:00

geoffkizer mentioned this pull request Jul 10, 2021

[main] Update dependencies from dotnet/efcore dotnet/runtime dotnet/aspnetcore#34033

Merged

karelz mentioned this pull request Jul 28, 2021

System.Net.Security test run killed on Mono Linux #56478

Closed

ghost locked as resolved and limited conversation to collaborators Aug 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

alnikola commented Jun 23, 2021

ghost commented Jun 23, 2021

ManickaP left a comment

ManickaP Jun 25, 2021

alnikola Jun 28, 2021

geoffkizer Jun 28, 2021

alnikola Jun 29, 2021 •

edited

Loading

alnikola Jul 1, 2021

geoffkizer Jul 1, 2021

alnikola commented Jun 28, 2021

ManickaP left a comment

geoffkizer Jul 1, 2021

alnikola Jul 1, 2021

geoffkizer left a comment

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

	ThrowRequestAborted(resetException);
	}

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY #54625

Conversation

alnikola commented Jun 23, 2021

ghost commented Jun 23, 2021

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP Jun 25, 2021

Choose a reason for hiding this comment

alnikola Jun 28, 2021

Choose a reason for hiding this comment

geoffkizer Jun 28, 2021

Choose a reason for hiding this comment

alnikola Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

alnikola Jul 1, 2021

Choose a reason for hiding this comment

geoffkizer Jul 1, 2021

Choose a reason for hiding this comment

alnikola commented Jun 28, 2021

ManickaP left a comment

Choose a reason for hiding this comment

geoffkizer Jul 1, 2021

Choose a reason for hiding this comment

alnikola Jul 1, 2021

Choose a reason for hiding this comment

geoffkizer left a comment

Choose a reason for hiding this comment

alnikola Jun 29, 2021 •

edited

Loading