From e2972bb1a7c4617337e37aea7378359759c422d4 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Tue, 24 Aug 2021 16:29:22 -0700 Subject: [PATCH 1/6] add Http3LoopbackConnection.ShutdownAsync and use in AcceptConnectionAsync --- .../Net/Http/Http3LoopbackConnection.cs | 35 +++++++++++++++++++ .../System/Net/Http/Http3LoopbackServer.cs | 10 ++++-- .../HttpClientHandlerTest.Cancellation.cs | 22 ++++++------ 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs index ae39fdc92e1373..cb65b22dd02702 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs @@ -36,7 +36,11 @@ internal sealed class Http3LoopbackConnection : GenericLoopbackConnection // This is specifically request streams, not control streams private readonly Dictionary _openStreams = new Dictionary(); + private Http3LoopbackStream _currentStream; + // We can't retrieve the stream ID after the stream is disposed, so store it separately + // Initialize it to -4 so that the firstInvalidStreamId calculation will work even if we never process a request + private long _currentStreamId = -4; private Http3LoopbackStream _inboundControlStream; // Inbound control stream from client private Http3LoopbackStream _outboundControlStream; // Our outbound control stream @@ -111,6 +115,7 @@ public async Task AcceptStreamAsync() { _openStreams.Add(checked((int)quicStream.StreamId), stream); _currentStream = stream; + _currentStreamId = quicStream.StreamId; } return stream; @@ -223,6 +228,8 @@ public override async Task HandleRequestAsync(HttpStatusCode st // We are about to close the connection, after we send the response. // So, send a GOAWAY frame now so the client won't inadvertantly try to reuse the connection. + // Note that in HTTP3 (unlike HTTP2) there is no strict ordering between the GOAWAY and the response below; + // so the client may race in processing them and we need to handle this. await _outboundControlStream.SendGoAwayFrameAsync(stream.StreamId + 4); await stream.SendResponseAsync(statusCode, headers, content).ConfigureAwait(false); @@ -232,6 +239,34 @@ public override async Task HandleRequestAsync(HttpStatusCode st return request; } + public async Task ShutdownAsync() + { + try + { + long firstInvalidStreamId = _currentStreamId + 4; + await _outboundControlStream.SendGoAwayFrameAsync(firstInvalidStreamId); + } + catch (QuicConnectionAbortedException abortException) when (abortException.ErrorCode == H3_NO_ERROR) + { + // Client must have closed the connection already because the HttpClientHandler instance was disposed. + // So nothing to do. + return; + } + catch (OperationCanceledException) + { + // If the client is closing the connection at the same time we are trying to send the GOAWAY above, + // this can result in OperationCanceledException from QuicStream.WriteAsync. + // See https://github.com/dotnet/runtime/issues/58078 + // I saw this consistently with GetAsync_EmptyResponseHeader_Success. + // To work around this, just eat the exception for now. + // Also, be aware of this issue as it will do weird things with OperationCanceledException and can + // make debugging this very confusing: https://github.com/dotnet/runtime/issues/58081 + return; + } + + await WaitForClientDisconnectAsync(); + } + // Wait for the client to close the connection, e.g. after we send a GOAWAY, or after the HttpClient is disposed. public async Task WaitForClientDisconnectAsync(bool refuseNewRequests = true) { diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs index 3ba40c46554397..6361194f0bbfb6 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs @@ -52,7 +52,7 @@ public override void Dispose() _cert.Dispose(); } - public override async Task EstablishGenericConnectionAsync() + private async Task EstablishHttp3ConnectionAsync() { QuicConnection con = await _listener.AcceptConnectionAsync().ConfigureAwait(false); Http3LoopbackConnection connection = new Http3LoopbackConnection(con); @@ -61,10 +61,16 @@ public override async Task EstablishGenericConnection return connection; } + public override async Task EstablishGenericConnectionAsync() + { + return await EstablishHttp3ConnectionAsync(); + } + public override async Task AcceptConnectionAsync(Func funcAsync) { - using GenericLoopbackConnection con = await EstablishGenericConnectionAsync().ConfigureAwait(false); + using Http3LoopbackConnection con = await EstablishHttp3ConnectionAsync().ConfigureAwait(false); await funcAsync(con).ConfigureAwait(false); + await con.ShutdownAsync(); } public override async Task HandleRequestAsync(HttpStatusCode statusCode = HttpStatusCode.OK, IList headers = null, string content = "") diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs index d4c4a571ad3517..f7d10cf7c63192 100644 --- a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs +++ b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs @@ -210,12 +210,9 @@ public async Task GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCance using (HttpClient client = CreateHttpClient()) { client.Timeout = Timeout.InfiniteTimeSpan; - var cts = new CancellationTokenSource(); await LoopbackServerFactory.CreateServerAsync(async (server, url) => { - var clientFinished = new TaskCompletionSource(); - Task serverTask = server.AcceptConnectionAsync(async connection => { var headers = new List(); @@ -227,16 +224,23 @@ await LoopbackServerFactory.CreateServerAsync(async (server, url) => await connection.ReadRequestDataAsync(); await connection.SendResponseAsync(HttpStatusCode.OK, headers: headers, isFinal: false); - await clientFinished.Task; + + await connection.WaitForCancellationAsync(); }); var req = new HttpRequestMessage(HttpMethod.Get, url) { Version = UseVersion }; req.Headers.ConnectionClose = connectionClose; - Task getResponse = client.SendAsync(TestAsync, req, HttpCompletionOption.ResponseHeadersRead, cts.Token); + Task getResponse = client.SendAsync(TestAsync, req, HttpCompletionOption.ResponseHeadersRead); await ValidateClientCancellationAsync(async () => { - HttpResponseMessage resp = await getResponse; + // This 'using' shouldn't be necessary in general. However, HTTP3 does not remove the request stream from the + // active stream table until the user disposes the response (or it gets finalized). + // This means the connection will fail to shut down promptly. + // See https://github.com/dotnet/runtime/issues/58072 + using HttpResponseMessage resp = await getResponse; + Stream respStream = await resp.Content.ReadAsStreamAsync(TestAsync); + using var cts = new CancellationTokenSource(); Task readTask = readOrCopyToAsync ? respStream.ReadAsync(new byte[1], 0, 1, cts.Token) : respStream.CopyToAsync(Stream.Null, 10, cts.Token); @@ -244,11 +248,7 @@ await ValidateClientCancellationAsync(async () => await readTask; }); - try - { - clientFinished.SetResult(true); - await serverTask; - } catch { } + await serverTask; }); } } From 657512c3b604da74d3915cf3f337a2426fdaf2d4 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 26 Aug 2021 19:58:14 -0700 Subject: [PATCH 2/6] simpler fix for GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCanceledQuickly test --- .../Http/HttpClientHandlerTest.Cancellation.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs index f7d10cf7c63192..7ae59fe98e54d2 100644 --- a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs +++ b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cancellation.cs @@ -210,9 +210,12 @@ public async Task GetAsync_CancelDuringResponseBodyReceived_Unbuffered_TaskCance using (HttpClient client = CreateHttpClient()) { client.Timeout = Timeout.InfiniteTimeSpan; + var cts = new CancellationTokenSource(); await LoopbackServerFactory.CreateServerAsync(async (server, url) => { + var clientFinished = new TaskCompletionSource(); + Task serverTask = server.AcceptConnectionAsync(async connection => { var headers = new List(); @@ -224,13 +227,12 @@ await LoopbackServerFactory.CreateServerAsync(async (server, url) => await connection.ReadRequestDataAsync(); await connection.SendResponseAsync(HttpStatusCode.OK, headers: headers, isFinal: false); - - await connection.WaitForCancellationAsync(); + await clientFinished.Task; }); var req = new HttpRequestMessage(HttpMethod.Get, url) { Version = UseVersion }; req.Headers.ConnectionClose = connectionClose; - Task getResponse = client.SendAsync(TestAsync, req, HttpCompletionOption.ResponseHeadersRead); + Task getResponse = client.SendAsync(TestAsync, req, HttpCompletionOption.ResponseHeadersRead, cts.Token); await ValidateClientCancellationAsync(async () => { // This 'using' shouldn't be necessary in general. However, HTTP3 does not remove the request stream from the @@ -238,9 +240,7 @@ await ValidateClientCancellationAsync(async () => // This means the connection will fail to shut down promptly. // See https://github.com/dotnet/runtime/issues/58072 using HttpResponseMessage resp = await getResponse; - Stream respStream = await resp.Content.ReadAsStreamAsync(TestAsync); - using var cts = new CancellationTokenSource(); Task readTask = readOrCopyToAsync ? respStream.ReadAsync(new byte[1], 0, 1, cts.Token) : respStream.CopyToAsync(Stream.Null, 10, cts.Token); @@ -248,7 +248,11 @@ await ValidateClientCancellationAsync(async () => await readTask; }); - await serverTask; + try + { + clientFinished.SetResult(true); + await serverTask; + } catch { } }); } } From b35d03d18a4b12ca610fa95fe6d022485c5ad23b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= <11718369+ManickaP@users.noreply.github.com> Date: Fri, 27 Aug 2021 14:47:10 +0200 Subject: [PATCH 3/6] Update src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs --- .../tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs index ac1ba7ebee4ed7..e54798becb1d89 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs @@ -29,7 +29,7 @@ public async Task IncompleteResponseStream_ResponseDropped_CancelsRequestToServe { if (UseVersion == HttpVersion30) { - // [ActiveIssue("https://github.com/dotnet/runtime/issues/53089")] + // [ActiveIssue("https://github.com/dotnet/runtime/issues/58234")] return; } From e1997efadffe90bc440b53830d8ac21bea358472 Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Tue, 24 Aug 2021 19:55:35 +0200 Subject: [PATCH 4/6] Enable IncompleteResponseStream_ResponseDropped_CancelsRequestToServer for H/3 (#58024) Fixes #53089 --- .../FunctionalTests/HttpClientHandlerTest.Finalization.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs index e54798becb1d89..a593fb066bc650 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Finalization.cs @@ -27,12 +27,6 @@ private static Task GetAndDropResponse(HttpClient client, Uri url) [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.SupportsAlpn))] public async Task IncompleteResponseStream_ResponseDropped_CancelsRequestToServer() { - if (UseVersion == HttpVersion30) - { - // [ActiveIssue("https://github.com/dotnet/runtime/issues/58234")] - return; - } - using (HttpClient client = CreateHttpClient()) { bool stopGCs = false; From 5b8bf671f3d3dbd5e683eda759efd787b324ca2d Mon Sep 17 00:00:00 2001 From: Natalia Kondratyeva Date: Tue, 24 Aug 2021 00:16:08 +0200 Subject: [PATCH 5/6] Re-enable GetAsync_LargeHeader_Success for H/3 (#57955) Fixes #55508 --- .../tests/FunctionalTests/HttpClientHandlerTest.Headers.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs index 590d888dc69ed0..da343ef56fc055 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs @@ -214,12 +214,6 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => [InlineData("RandomCustomHeader", 12345)] public async Task GetAsync_LargeHeader_Success(string headerName, int headerValueLength) { - if (UseVersion == HttpVersion.Version30) - { - // [ActiveIssue("https://github.com/dotnet/runtime/issues/55508")] - return; - } - var rand = new Random(42); string headerValue = string.Concat(Enumerable.Range(0, headerValueLength).Select(_ => (char)('A' + rand.Next(26)))); From 912bce5d18b7da202351f9cff113bf68ee1f4c75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= <11718369+ManickaP@users.noreply.github.com> Date: Mon, 30 Aug 2021 13:56:27 +0200 Subject: [PATCH 6/6] Reenabled test (#58041) --- .../tests/System/Net/Http/HttpClientHandlerTest.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs index 17fcd64ee26d80..0764022f3a8fa8 100644 --- a/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs +++ b/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs @@ -262,9 +262,9 @@ await LoopbackServer.CreateClientAndServerAsync(async proxyUri => public static IEnumerable SecureAndNonSecure_IPBasedUri_MemberData() => from address in new[] { IPAddress.Loopback, IPAddress.IPv6Loopback } - from useSsl in BoolValues + from useSsl in BoolValues // we could not create SslStream in browser, [ActiveIssue("https://github.com/dotnet/runtime/issues/37669", TestPlatforms.Browser)] - where PlatformDetection.IsNotBrowser || !useSsl + where PlatformDetection.IsNotBrowser || !useSsl select new object[] { address, useSsl }; [Theory] @@ -951,12 +951,6 @@ public async Task ReadAsStreamAsync_HandlerProducesWellBehavedResponseStream(boo return; } - if (UseVersion == HttpVersion30 && (chunked is null || chunked is false)) - { - // [ActiveIssue("https://github.com/dotnet/runtime/issues/53087")] - return; - } - await LoopbackServerFactory.CreateClientAndServerAsync(async uri => { var request = new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion };