From: Geoff Kizer Date: Tue, 17 Aug 2021 19:31:26 +0000 (-0700) Subject: don't dispose client control stream before closing connection (#57223) X-Git-Tag: accepted/tizen/unified/20220110.054933~308 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=02ab8d2d6513a4934b70ae5ff7765a0c803e9f4f;p=platform%2Fupstream%2Fdotnet%2Fruntime.git don't dispose client control stream before closing connection (#57223) * don't dispose client control stream before closing connection Co-authored-by: Geoffrey Kizer --- diff --git a/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs index 492aa58..ae39fdc 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.Text; using System.Threading.Tasks; using System.Linq; using System.Net.Http.Functional.Tests; +using Xunit; namespace System.Net.Test.Common { @@ -32,10 +33,13 @@ namespace System.Net.Test.Common 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 bool _closed; + + private Http3LoopbackStream _inboundControlStream; // Inbound control stream from client + private Http3LoopbackStream _outboundControlStream; // Our outbound control stream public Http3LoopbackConnection(QuicConnection connection) { @@ -44,23 +48,30 @@ namespace System.Net.Test.Common 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(); - } - - //_connection.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. + _connection.Dispose(); + + // Dispose control streams so that we release their handles too. + _inboundControlStream?.Dispose(); + _outboundControlStream?.Dispose(); +#endif } public async Task CloseAsync(long errorCode) { await _connection.CloseAsync(errorCode).ConfigureAwait(false); - _closed = true; } public Http3LoopbackStream OpenUnidirectionalStream() @@ -96,23 +107,50 @@ namespace System.Net.Test.Common 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 @@ namespace System.Net.Test.Common 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 @@ namespace System.Net.Test.Common // 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); @@ -221,6 +259,10 @@ namespace System.Net.Test.Common } } + // 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 dca12ee..2e0047e 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 @@ -113,12 +113,6 @@ namespace System.Net.Http return; } - if (_clientControl != null) - { - _clientControl.Dispose(); - _clientControl = null; - } - if (_connection != null) { // Close the QuicConnection in the background. @@ -146,6 +140,13 @@ namespace System.Net.Http { 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 c2594cf..7487a95 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 @@ namespace System.Net.Quic.Implementations.Mock 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; + } + } + internal override X509Certificate? RemoteCertificate => null; // Constructor for outbound connections 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 2c3d50a..588da85 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 @@ -86,6 +86,11 @@ namespace System.Net.Quic.Implementations.Mock 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) { @@ -137,6 +142,11 @@ namespace System.Net.Quic.Implementations.Mock throw new NotSupportedException(); } + if (_connection.ConnectionError is long connectonError) + { + throw new QuicConnectionAbortedException(connectonError); + } + long errorCode = _isInitiator ? _streamState._inboundWriteErrorCode : _streamState._outboundWriteErrorCode; if (errorCode != 0) {