From 0ade6f804312ff75c358ea0adbdee05d561bb279 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 18 Apr 2024 18:40:26 +0200 Subject: [PATCH] Fix data race leading to a deadlock. --- .../HttpClientHandlerTest.Http3.cs | 3 ++ .../src/System/Net/Quic/QuicConnection.cs | 11 +++++-- .../src/System/Net/Quic/QuicStream.cs | 31 +++++++++++++++++-- 3 files changed, 40 insertions(+), 5 deletions(-) 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 16abc260386b42..1b9e2aef9d44d6 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs @@ -1098,7 +1098,10 @@ public async Task ConnectionAttemptCanceled_AuthorityNotBlocked() } catch (Exception ex) // Ignore exception and continue until a viable connection is established. { + System.Console.WriteLine(ex.ToString()); _output.WriteLine(ex.ToString()); + var qe = Assert.IsType(ex); + Assert.Equal(QuicError.ConnectionAborted, qe.QuicError); } } await connection.HandleRequestAsync(); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 315352d2797bb4..73a252b14b9d90 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -413,7 +413,7 @@ public async ValueTask OpenOutboundStreamAsync(QuicStreamType type, stream = new QuicStream(_handle, type, _defaultStreamErrorCode); await stream.StartAsync(cancellationToken).ConfigureAwait(false); } - catch + catch (Exception ex) { if (stream is not null) { @@ -422,8 +422,15 @@ public async ValueTask OpenOutboundStreamAsync(QuicStreamType type, // Propagate ODE if disposed in the meantime. ObjectDisposedException.ThrowIf(_disposed == 1, this); + + // In case of an incoming race when the connection is closed by the peer just before we open the stream, + // we receive QUIC_STATUS_ABORTED from MsQuic, but we don't know how the connection was closed. To + // distinguish this case, we throw ConnectionAborted without ApplicationErrorCode. In such a case, we + // can expect the connection close exception to be already reported on the connection level (or very soon). + bool connectionAbortedByPeer = ex is QuicException qe && qe.QuicError == QuicError.ConnectionAborted && qe.ApplicationErrorCode is null; + // Propagate connection error if present. - if (_acceptQueue.Reader.Completion.IsFaulted) + if (_acceptQueue.Reader.Completion.IsFaulted || connectionAbortedByPeer) { await _acceptQueue.Reader.Completion.ConfigureAwait(false); } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index 06515f3310bf7f..38673b2817bb88 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -162,13 +162,24 @@ internal unsafe QuicStream(MsQuicContextSafeHandle connectionHandle, QuicStreamT try { QUIC_HANDLE* handle; - ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.StreamOpen( + int status = MsQuicApi.Api.StreamOpen( connectionHandle, type == QuicStreamType.Unidirectional ? QUIC_STREAM_OPEN_FLAGS.UNIDIRECTIONAL : QUIC_STREAM_OPEN_FLAGS.NONE, &NativeCallback, (void*)GCHandle.ToIntPtr(context), - &handle), - "StreamOpen failed"); + &handle); + + if (status == QUIC_STATUS_ABORTED) + { + // Connection has been closed by the peer (either at transport or application level), + // we won't be receiving any event callback for shutdown on this stream, so we don't + // necessarily know which error to report. So we throw an exception which we can distinguish + // at the caller (ConnectionAborted normally has App error code) and throw the correct + // exception from there. + throw new QuicException(QuicError.ConnectionAborted, null, ""); + } + + ThrowHelper.ThrowIfMsQuicError(status, "StreamOpen failed"); _handle = new MsQuicContextSafeHandle(handle, context, SafeHandleType.Stream, connectionHandle); _handle.Disposable = _sendBuffers; } @@ -244,6 +255,20 @@ internal ValueTask StartAsync(CancellationToken cancellationToken = default) int status = MsQuicApi.Api.StreamStart( _handle, QUIC_STREAM_START_FLAGS.SHUTDOWN_ON_FAIL | QUIC_STREAM_START_FLAGS.INDICATE_PEER_ACCEPT); + + if (status == QUIC_STATUS_ABORTED) + { + // Connection has been closed by the peer (either at transport or application level), + // we won't be receiving any event callback for shutdown on this stream, so we don't + // necessarily know which error to report. So we throw an exception which we can distinguish + // at the caller (ConnectionAborted normally has App error code) and throw the correct + // exception from there. + // + // Also, avoid setting _startedTcs so that we don't try to wait for SHUTDOWN_COMPLETE + // in DisposeAsync(). + throw new QuicException(QuicError.ConnectionAborted, null, ""); + } + if (ThrowHelper.TryGetStreamExceptionForMsQuicStatus(status, out Exception? exception)) { _startedTcs.TrySetException(exception);