From 79f755d287f629a7654729aea947393866a109da Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 11 Aug 2021 10:04:14 -0700 Subject: [PATCH 1/4] don't dispose client control stream before closing connection --- .../Net/Http/Http3LoopbackConnection.cs | 74 +++++++++++++++---- .../SocketsHttpHandler/Http3Connection.cs | 13 ++-- .../Implementations/Mock/MockConnection.cs | 14 ++++ .../Quic/Implementations/Mock/MockStream.cs | 10 +++ 4 files changed, 89 insertions(+), 22 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs index ca53bbe2068257..5d954b44c3c197 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs @@ -9,6 +9,7 @@ using System.Threading.Tasks; using System.Linq; using System.Net.Http.Functional.Tests; +using Xunit; namespace System.Net.Test.Common { @@ -32,9 +33,14 @@ internal sealed class Http3LoopbackConnection : GenericLoopbackConnection public const long H3_VERSION_FALLBACK = 0x110; private readonly QuicConnection _connection; + + // This is specifically request streams, not control streams private readonly Dictionary _openStreams = new Dictionary(); - private Http3LoopbackStream _controlStream; // Our outbound control stream private Http3LoopbackStream _currentStream; + + private Http3LoopbackStream _inboundControlStream; // Inbound control stream from client + private Http3LoopbackStream _outboundControlStream; // Our outbound control stream + private bool _closed; public Http3LoopbackConnection(QuicConnection connection) @@ -44,21 +50,26 @@ public Http3LoopbackConnection(QuicConnection connection) public override void Dispose() { + // Close any remaining request streams (but NOT control streams, as these should not be closed while the connection is open) foreach (Http3LoopbackStream stream in _openStreams.Values) { stream.Dispose(); } - if (!_closed) - { - // CloseAsync(H3_INTERNAL_ERROR).GetAwaiter().GetResult(); - } + // Dispose the connection + // If we already waited for graceful shutdown from the client, then the connection is already closed and this will simply release the handle. + // If not, then this will silently abort the connection. + _connection.Dispose(); - //_connection.Dispose(); + // Dispose control streams so that we release their handles too. + _inboundControlStream?.Dispose(); + _outboundControlStream?.Dispose(); } public async Task CloseAsync(long errorCode) { + Debug.Assert(!_closed); + await _connection.CloseAsync(errorCode).ConfigureAwait(false); _closed = true; } @@ -96,23 +107,50 @@ public async Task AcceptStreamAsync() QuicStream quicStream = await _connection.AcceptStreamAsync().ConfigureAwait(false); var stream = new Http3LoopbackStream(quicStream); - _openStreams.Add(checked((int)quicStream.StreamId), stream); - _currentStream = stream; + if (quicStream.CanWrite) + { + _openStreams.Add(checked((int)quicStream.StreamId), stream); + _currentStream = stream; + } return stream; } + private async Task HandleControlStreamAsync(Http3LoopbackStream controlStream) + { + if (_inboundControlStream is not null) + { + throw new Exception("Received second control stream from client???"); + } + + long? streamType = await controlStream.ReadIntegerAsync(); + Assert.Equal(Http3LoopbackStream.ControlStream, streamType); + + List<(long settingId, long settingValue)> settings = await controlStream.ReadSettingsAsync(); + (long settingId, long settingValue) = Assert.Single(settings); + + Assert.Equal(Http3LoopbackStream.MaxHeaderListSize, settingId); + + _inboundControlStream = controlStream; + } + + // This will automatically handle the control stream, including validating its contents public async Task AcceptRequestStreamAsync() { Http3LoopbackStream stream; - do + while (true) { stream = await AcceptStreamAsync().ConfigureAwait(false); - } - while (!stream.CanWrite); // skip control stream. - return stream; + if (stream.CanWrite) + { + return stream; + } + + // Must be the control stream + await HandleControlStreamAsync(stream); + } } public async Task<(Http3LoopbackStream clientControlStream, Http3LoopbackStream requestStream)> AcceptControlAndRequestStreamAsync() @@ -141,9 +179,9 @@ public async Task AcceptRequestStreamAsync() public async Task EstablishControlStreamAsync() { - _controlStream = OpenUnidirectionalStream(); - await _controlStream.SendUnidirectionalStreamTypeAsync(Http3LoopbackStream.ControlStream); - await _controlStream.SendSettingsFrameAsync(); + _outboundControlStream = OpenUnidirectionalStream(); + await _outboundControlStream.SendUnidirectionalStreamTypeAsync(Http3LoopbackStream.ControlStream); + await _outboundControlStream.SendSettingsFrameAsync(); } public override async Task ReadRequestBodyAsync() @@ -185,7 +223,7 @@ 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. - await _controlStream.SendGoAwayFrameAsync(stream.StreamId + 4); + await _outboundControlStream.SendGoAwayFrameAsync(stream.StreamId + 4); await stream.SendResponseAsync(statusCode, headers, content).ConfigureAwait(false); @@ -216,6 +254,10 @@ public async Task WaitForClientDisconnectAsync() } } + // The client's control stream should throw QuicConnectionAbortedException, indicating that it was + // aborted because the connection was closed (and was not explicitly closed or aborted prior to the connection being closed) + await Assert.ThrowsAsync(async () => await _inboundControlStream.ReadFrameAsync()); + await CloseAsync(H3_NO_ERROR); } 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 6b2012d6df384c..daebdf24f4b136 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 @@ -118,12 +118,6 @@ private void CheckForShutdown() return; } - if (_clientControl != null) - { - _clientControl.Dispose(); - _clientControl = null; - } - if (_connection != null) { // Close the QuicConnection in the background. @@ -151,6 +145,13 @@ private void CheckForShutdown() { Trace($"{nameof(QuicConnection)} failed to dispose: {ex}"); } + + if (_clientControl != null) + { + _clientControl.Dispose(); + _clientControl = null; + } + }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs index 1e8fd20f066e5d..6d0516be2eee1e 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs @@ -29,6 +29,20 @@ internal sealed class MockConnection : QuicConnectionProvider internal PeerStreamLimit? LocalStreamLimit => _isClient ? _state?._clientStreamLimit : _state?._serverStreamLimit; internal PeerStreamLimit? RemoteStreamLimit => _isClient ? _state?._serverStreamLimit : _state?._clientStreamLimit; + internal long? ConnectionError + { + get + { + long? errorCode = _isClient ? _state?._serverErrorCode : _state?._clientErrorCode; + if (errorCode == -1) + { + errorCode = null; + } + + return errorCode; + } + } + // Constructor for outbound connections internal MockConnection(EndPoint? remoteEndPoint, SslClientAuthenticationOptions? sslClientAuthenticationOptions, IPEndPoint? localEndPoint = null, int maxUnidirectionalStreams = 100, int maxBidirectionalStreams = 100) { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockStream.cs index fde0eab97d197b..28c0d5b413d2a0 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockStream.cs @@ -70,6 +70,11 @@ internal override async ValueTask ReadAsync(Memory buffer, Cancellati int bytesRead = await streamBuffer.ReadAsync(buffer, cancellationToken).ConfigureAwait(false); if (bytesRead == 0) { + if (_connection.ConnectionError is long connectonError) + { + throw new QuicConnectionAbortedException(connectonError); + } + long errorCode = _isInitiator ? _streamState._inboundReadErrorCode : _streamState._outboundReadErrorCode; if (errorCode != 0) { @@ -121,6 +126,11 @@ internal override async ValueTask WriteAsync(ReadOnlyMemory buffer, bool e throw new NotSupportedException(); } + if (_connection.ConnectionError is long connectonError) + { + throw new QuicConnectionAbortedException(connectonError); + } + long errorCode = _isInitiator ? _streamState._inboundWriteErrorCode : _streamState._outboundWriteErrorCode; if (errorCode != 0) { From c9c4c82c9968fb33aac102db1b056a6338e2dfc0 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 11 Aug 2021 17:45:09 -0700 Subject: [PATCH 2/4] fix spacing issue cause by github merge --- .../src/System/Net/Quic/Implementations/Mock/MockConnection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs index d8cadddb8d15fe..7487a958db91f9 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs @@ -42,7 +42,7 @@ internal long? ConnectionError return errorCode; } } - + internal override X509Certificate? RemoteCertificate => null; // Constructor for outbound connections From ae3bdbe948dccbea123834b098c21c9e608f0e74 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Tue, 17 Aug 2021 08:33:27 -0700 Subject: [PATCH 3/4] Don't dispose the loopback connection in Http3LoopbackConnection.Dispose --- .../Common/tests/System/Net/Http/Http3LoopbackConnection.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs index fd4a851c712f2e..d8d1e2a2f464df 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs @@ -56,6 +56,10 @@ public override void Dispose() stream.Dispose(); } +// We don't dispose the connection currently, because this causes races when the server connection is closed before +// the client has received and handled all response data. +// See discussion in https://github.com/dotnet/runtime/pull/57223#discussion_r687447832 +#if false // Dispose the connection // If we already waited for graceful shutdown from the client, then the connection is already closed and this will simply release the handle. // If not, then this will silently abort the connection. @@ -64,6 +68,7 @@ public override void Dispose() // Dispose control streams so that we release their handles too. _inboundControlStream?.Dispose(); _outboundControlStream?.Dispose(); +#endif } public async Task CloseAsync(long errorCode) From 8331cdac02502a27b51bcbe16a7f6845bf842ab4 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Tue, 17 Aug 2021 09:31:59 -0700 Subject: [PATCH 4/4] remove _closed flag and related assert --- .../Common/tests/System/Net/Http/Http3LoopbackConnection.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs index d8d1e2a2f464df..ae39fdc92e1373 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs @@ -41,8 +41,6 @@ internal sealed class Http3LoopbackConnection : GenericLoopbackConnection private Http3LoopbackStream _inboundControlStream; // Inbound control stream from client private Http3LoopbackStream _outboundControlStream; // Our outbound control stream - private bool _closed; - public Http3LoopbackConnection(QuicConnection connection) { _connection = connection; @@ -73,10 +71,7 @@ public override void Dispose() public async Task CloseAsync(long errorCode) { - Debug.Assert(!_closed); - await _connection.CloseAsync(errorCode).ConfigureAwait(false); - _closed = true; } public Http3LoopbackStream OpenUnidirectionalStream()