From d3c77a57c2763f6907fb562130f2f156f8a713a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marie=20P=C3=ADchov=C3=A1?= <11718369+ManickaP@users.noreply.github.com> Date: Tue, 27 Apr 2021 18:13:22 +0200 Subject: [PATCH] Fixed non-decreasing count of stream on H3 connection. (#51742) * Fixed HTTP/3 client hang after subsequent 100 streams. Fixed testing H/3 loopback stream to properly close. Otherwise, the stream will end up in a bad state and the connection will eventually run out of available streams. * Added assert. --- .../System/Net/Http/Http3LoopbackStream.cs | 1 + .../SocketsHttpHandler/Http3Connection.cs | 24 ++++++++---- .../SocketsHttpHandler/Http3RequestStream.cs | 2 +- .../HttpClientHandlerTest.Http3.cs | 37 +++++++++++++++++++ .../Implementations/MsQuic/MsQuicStream.cs | 2 +- 5 files changed, 57 insertions(+), 9 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs index e2fb12e5ff9ae..faccfab64b27e 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackStream.cs @@ -227,6 +227,7 @@ public async Task SendResponseBodyAsync(byte[] content, bool isFinal = true) if (isFinal) { await ShutdownSendAsync().ConfigureAwait(false); + await _stream.ShutdownCompleted().ConfigureAwait(false); Dispose(); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs index 4e022598ee564..ca6dd5df9fc83 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs @@ -285,7 +285,7 @@ private void CancelWaiters() } } - // TODO: how do we get this event? + // TODO: how do we get this event? -> HandleEventStreamsAvailable reports currently available Uni/Bi streams private void OnMaximumStreamCountIncrease(long newMaximumStreamCount) { lock (SyncObj) @@ -295,15 +295,23 @@ private void OnMaximumStreamCountIncrease(long newMaximumStreamCount) return; } - _requestStreamsRemaining += (newMaximumStreamCount - _maximumRequestStreams); + IncreaseRemainingStreamCount(newMaximumStreamCount - _maximumRequestStreams); _maximumRequestStreams = newMaximumStreamCount; + } + } + + private void IncreaseRemainingStreamCount(long delta) + { + Debug.Assert(Monitor.IsEntered(SyncObj)); + Debug.Assert(delta > 0); - while (_requestStreamsRemaining != 0 && _waitingRequests.TryDequeue(out TaskCompletionSourceWithCancellation? tcs)) + _requestStreamsRemaining += delta; + + while (_requestStreamsRemaining != 0 && _waitingRequests.TryDequeue(out TaskCompletionSourceWithCancellation? tcs)) + { + if (tcs.TrySetResult(true)) { - if (tcs.TrySetResult(true)) - { - --_requestStreamsRemaining; - } + --_requestStreamsRemaining; } } } @@ -406,6 +414,8 @@ public void RemoveStream(QuicStream stream) bool removed = _activeRequests.Remove(stream); Debug.Assert(removed == true); + IncreaseRemainingStreamCount(1); + if (ShuttingDown) { CheckForShutdown(); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs index d2c79de7d28e4..970890ddcd9fb 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs @@ -312,7 +312,7 @@ private async Task ReadResponseAsync(CancellationToken cancellationToken) { if (NetEventSource.Log.IsEnabled()) { - Trace($"Expected HEADERS as first response frame; recieved {frameType}."); + Trace($"Expected HEADERS as first response frame; received {frameType}."); } throw new HttpRequestException(SR.net_http_invalid_response); } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs index 230a9a6d40da1..5efbfcfcd0f6d 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs @@ -78,6 +78,43 @@ public async Task ClientSettingsReceived_Success(int headerSizeLimit) await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000); } + [Theory] + [InlineData(100)] + public async Task SendMoreThanStreamLimitRequests_Succeeds(int streamLimit) + { + using Http3LoopbackServer server = CreateHttp3LoopbackServer(); + + Task serverTask = Task.Run(async () => + { + using Http3LoopbackConnection connection = (Http3LoopbackConnection)await server.EstablishGenericConnectionAsync(); + for (int i = 0; i < streamLimit + 1; ++i) + { + using Http3LoopbackStream stream = await connection.AcceptRequestStreamAsync(); + await stream.HandleRequestAsync(); + } + }); + + Task clientTask = Task.Run(async () => + { + using HttpClient client = CreateHttpClient(); + + for (int i = 0; i < streamLimit + 1; ++i) + { + using HttpRequestMessage request = new() + { + Method = HttpMethod.Get, + RequestUri = server.Address, + Version = HttpVersion30, + VersionPolicy = HttpVersionPolicy.RequestVersionExact + }; + using var response = await client.SendAsync(request).WaitAsync(TimeSpan.FromSeconds(10)); + } + }); + + await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000); + } + + [Fact] public async Task ReservedFrameType_Throws() { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs index ccc18c9d225dc..4672f9f8be37e 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs @@ -298,7 +298,7 @@ internal override async ValueTask ReadAsync(Memory destination, Cance } }, _state); - // TODO there could potentially be a perf gain by storing the buffer from the inital read + // TODO there could potentially be a perf gain by storing the buffer from the initial read // This reduces the amount of async calls, however it makes it so MsQuic holds onto the buffers // longer than it needs to. We will need to benchmark this. int length = (int)await _state.ReceiveResettableCompletionSource.GetValueTask().ConfigureAwait(false);