From 3c59110cc4df97925c9a41880cee0218c1ff0e22 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Wed, 23 Jun 2021 19:49:11 +0200 Subject: [PATCH 1/5] Http2Stream throws a wrapped Http2ConnectionException on GO_AWAY --- .../Http/SocketsHttpHandler/Http2Stream.cs | 9 +++++ .../HttpClientHandlerTest.Http2.cs | 37 ++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index d7d7a88970375..fc12a8949130b 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -1275,6 +1275,15 @@ private async ValueTask SendDataAsync(ReadOnlyMemory buffer, CancellationT await _connection.SendStreamDataAsync(StreamId, current, flush, _requestBodyCancellationSource.Token).ConfigureAwait(false); } } + catch (Exception) + { + if (_resetException is Exception resetException) + { + ThrowRequestAborted(resetException); + } + + throw; + } finally { linkedRegistration.Dispose(); diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 668d194ebef1c..f2e54ca87b738 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -903,6 +903,39 @@ await Assert.ThrowsAnyAsync(() => } } + [ConditionalFact(nameof(SupportsAlpn))] + public async Task GoAwayFrame_RequestWithBody_ServerDisconnect_AbortStreamsAndThrowIOException() + { + using (Http2LoopbackServer server = Http2LoopbackServer.CreateServer()) + using (HttpClient client = CreateHttpClient()) + { + var request = new HttpRequestMessage(HttpMethod.Post, server.Address); + request.Version = new Version(2, 0); + var content = new string('*', 300); + var stream = new CustomContent.SlowTestStream(Encoding.UTF8.GetBytes(content), null, count: 60); + request.Content = new CustomContent(stream); + + Task sendTask = client.SendAsync(request); + + Http2LoopbackConnection connection = await server.EstablishConnectionAsync(); + (int streamId, _) = await connection.ReadAndParseRequestHeaderAsync(readBody: false); + await connection.SendDefaultResponseHeadersAsync(streamId); + + await connection.SendGoAway(0, errorCode: ProtocolErrors.PROTOCOL_ERROR); + + // Expect client to detect that server has disconnected and throw an exception + var exception = await Assert.ThrowsAnyAsync(() => + new Task[] + { + sendTask + }.WhenAllOrAnyFailed(TestHelper.PassingTestTimeoutMilliseconds)); + + Assert.IsType(exception.InnerException); + Assert.NotNull(exception.InnerException.InnerException); + Assert.Contains("HTTP/2 error code 'PROTOCOL_ERROR'", exception.InnerException.InnerException.Message); + } + } + [ConditionalFact(nameof(SupportsAlpn))] public async Task GoAwayFrame_UnprocessedStreamFirstRequestFinishedFirst_RequestRestarted() { @@ -2790,8 +2823,8 @@ public async Task PostAsyncDuplex_ServerResetsStream_Throws() // Trying to read on the response stream should fail now, and client should ignore any data received await AssertProtocolErrorForIOExceptionAsync(SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId), ProtocolErrors.ENHANCE_YOUR_CALM); - // Attempting to write on the request body should now fail with OperationCanceledException. - Exception e = await Assert.ThrowsAnyAsync(async () => { await SendAndReceiveRequestDataAsync(contentBytes, requestStream, connection, streamId); }); + // Attempting to write on the request body should now fail with IOException. + Exception e = await Assert.ThrowsAnyAsync(async () => { await SendAndReceiveRequestDataAsync(contentBytes, requestStream, connection, streamId); }); // Propagate the exception to the request stream serialization task. // This allows the request processing to complete. From cc922ae8243d233f4421611ca683df8b1bd3c34a Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Mon, 28 Jun 2021 12:05:39 +0200 Subject: [PATCH 2/5] Catching only OperationCanceledException with the specific token --- .../src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs | 2 +- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index fc12a8949130b..4f05fc3fbd565 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -1275,7 +1275,7 @@ private async ValueTask SendDataAsync(ReadOnlyMemory buffer, CancellationT await _connection.SendStreamDataAsync(StreamId, current, flush, _requestBodyCancellationSource.Token).ConfigureAwait(false); } } - catch (Exception) + catch (OperationCanceledException e) when (e.CancellationToken == _requestBodyCancellationSource.Token) { if (_resetException is Exception resetException) { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index f2e54ca87b738..54c5d79774e68 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -921,6 +921,7 @@ public async Task GoAwayFrame_RequestWithBody_ServerDisconnect_AbortStreamsAndTh (int streamId, _) = await connection.ReadAndParseRequestHeaderAsync(readBody: false); await connection.SendDefaultResponseHeadersAsync(streamId); + Debugger.Launch(); await connection.SendGoAway(0, errorCode: ProtocolErrors.PROTOCOL_ERROR); // Expect client to detect that server has disconnected and throw an exception @@ -932,7 +933,7 @@ public async Task GoAwayFrame_RequestWithBody_ServerDisconnect_AbortStreamsAndTh Assert.IsType(exception.InnerException); Assert.NotNull(exception.InnerException.InnerException); - Assert.Contains("HTTP/2 error code 'PROTOCOL_ERROR'", exception.InnerException.InnerException.Message); + Assert.Contains("PROTOCOL_ERROR", exception.InnerException.InnerException.Message); } } From be882996a1db564621a1fc39b29caa50f7be602b Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Mon, 28 Jun 2021 12:07:01 +0200 Subject: [PATCH 3/5] _canRetry is checked --- .../src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index 4f05fc3fbd565..3612bf6f90fa4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -1279,6 +1279,11 @@ private async ValueTask SendDataAsync(ReadOnlyMemory buffer, CancellationT { if (_resetException is Exception resetException) { + if (_canRetry) + { + ThrowRetry(SR.net_http_request_aborted, resetException); + } + ThrowRequestAborted(resetException); } From 6b028a711b5890975db245fc010e4149cf754ef5 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev Date: Tue, 29 Jun 2021 15:10:45 +0200 Subject: [PATCH 4/5] Fix test --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 54c5d79774e68..e61e728ede997 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -921,7 +921,6 @@ public async Task GoAwayFrame_RequestWithBody_ServerDisconnect_AbortStreamsAndTh (int streamId, _) = await connection.ReadAndParseRequestHeaderAsync(readBody: false); await connection.SendDefaultResponseHeadersAsync(streamId); - Debugger.Launch(); await connection.SendGoAway(0, errorCode: ProtocolErrors.PROTOCOL_ERROR); // Expect client to detect that server has disconnected and throw an exception From 949acf65bd3498bc7efb48d6e42234073846d030 Mon Sep 17 00:00:00 2001 From: Alexander Nikolaev <55398552+alnikola@users.noreply.github.com> Date: Thu, 1 Jul 2021 18:03:57 +0200 Subject: [PATCH 5/5] Add lock around _resetException and _canRetry --- .../Net/Http/SocketsHttpHandler/Http2Stream.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index 3612bf6f90fa4..0300d3c88a6b6 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -1277,14 +1277,17 @@ private async ValueTask SendDataAsync(ReadOnlyMemory buffer, CancellationT } catch (OperationCanceledException e) when (e.CancellationToken == _requestBodyCancellationSource.Token) { - if (_resetException is Exception resetException) + lock (SyncObject) { - if (_canRetry) + if (_resetException is Exception resetException) { - ThrowRetry(SR.net_http_request_aborted, resetException); - } + if (_canRetry) + { + ThrowRetry(SR.net_http_request_aborted, resetException); + } - ThrowRequestAborted(resetException); + ThrowRequestAborted(resetException); + } } throw;