Skip to content

Commit 0ade6f8

Browse files
committed
Fix data race leading to a deadlock.
1 parent 9068070 commit 0ade6f8

File tree

3 files changed

+40
-5
lines changed

3 files changed

+40
-5
lines changed

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,7 +1098,10 @@ public async Task ConnectionAttemptCanceled_AuthorityNotBlocked()
10981098
}
10991099
catch (Exception ex) // Ignore exception and continue until a viable connection is established.
11001100
{
1101+
System.Console.WriteLine(ex.ToString());
11011102
_output.WriteLine(ex.ToString());
1103+
var qe = Assert.IsType<QuicException>(ex);
1104+
Assert.Equal(QuicError.ConnectionAborted, qe.QuicError);
11021105
}
11031106
}
11041107
await connection.HandleRequestAsync();

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ public async ValueTask<QuicStream> OpenOutboundStreamAsync(QuicStreamType type,
413413
stream = new QuicStream(_handle, type, _defaultStreamErrorCode);
414414
await stream.StartAsync(cancellationToken).ConfigureAwait(false);
415415
}
416-
catch
416+
catch (Exception ex)
417417
{
418418
if (stream is not null)
419419
{
@@ -422,8 +422,15 @@ public async ValueTask<QuicStream> OpenOutboundStreamAsync(QuicStreamType type,
422422

423423
// Propagate ODE if disposed in the meantime.
424424
ObjectDisposedException.ThrowIf(_disposed == 1, this);
425+
426+
// In case of an incoming race when the connection is closed by the peer just before we open the stream,
427+
// we receive QUIC_STATUS_ABORTED from MsQuic, but we don't know how the connection was closed. To
428+
// distinguish this case, we throw ConnectionAborted without ApplicationErrorCode. In such a case, we
429+
// can expect the connection close exception to be already reported on the connection level (or very soon).
430+
bool connectionAbortedByPeer = ex is QuicException qe && qe.QuicError == QuicError.ConnectionAborted && qe.ApplicationErrorCode is null;
431+
425432
// Propagate connection error if present.
426-
if (_acceptQueue.Reader.Completion.IsFaulted)
433+
if (_acceptQueue.Reader.Completion.IsFaulted || connectionAbortedByPeer)
427434
{
428435
await _acceptQueue.Reader.Completion.ConfigureAwait(false);
429436
}

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,13 +162,24 @@ internal unsafe QuicStream(MsQuicContextSafeHandle connectionHandle, QuicStreamT
162162
try
163163
{
164164
QUIC_HANDLE* handle;
165-
ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.StreamOpen(
165+
int status = MsQuicApi.Api.StreamOpen(
166166
connectionHandle,
167167
type == QuicStreamType.Unidirectional ? QUIC_STREAM_OPEN_FLAGS.UNIDIRECTIONAL : QUIC_STREAM_OPEN_FLAGS.NONE,
168168
&NativeCallback,
169169
(void*)GCHandle.ToIntPtr(context),
170-
&handle),
171-
"StreamOpen failed");
170+
&handle);
171+
172+
if (status == QUIC_STATUS_ABORTED)
173+
{
174+
// Connection has been closed by the peer (either at transport or application level),
175+
// we won't be receiving any event callback for shutdown on this stream, so we don't
176+
// necessarily know which error to report. So we throw an exception which we can distinguish
177+
// at the caller (ConnectionAborted normally has App error code) and throw the correct
178+
// exception from there.
179+
throw new QuicException(QuicError.ConnectionAborted, null, "");
180+
}
181+
182+
ThrowHelper.ThrowIfMsQuicError(status, "StreamOpen failed");
172183
_handle = new MsQuicContextSafeHandle(handle, context, SafeHandleType.Stream, connectionHandle);
173184
_handle.Disposable = _sendBuffers;
174185
}
@@ -244,6 +255,20 @@ internal ValueTask StartAsync(CancellationToken cancellationToken = default)
244255
int status = MsQuicApi.Api.StreamStart(
245256
_handle,
246257
QUIC_STREAM_START_FLAGS.SHUTDOWN_ON_FAIL | QUIC_STREAM_START_FLAGS.INDICATE_PEER_ACCEPT);
258+
259+
if (status == QUIC_STATUS_ABORTED)
260+
{
261+
// Connection has been closed by the peer (either at transport or application level),
262+
// we won't be receiving any event callback for shutdown on this stream, so we don't
263+
// necessarily know which error to report. So we throw an exception which we can distinguish
264+
// at the caller (ConnectionAborted normally has App error code) and throw the correct
265+
// exception from there.
266+
//
267+
// Also, avoid setting _startedTcs so that we don't try to wait for SHUTDOWN_COMPLETE
268+
// in DisposeAsync().
269+
throw new QuicException(QuicError.ConnectionAborted, null, "");
270+
}
271+
247272
if (ThrowHelper.TryGetStreamExceptionForMsQuicStatus(status, out Exception? exception))
248273
{
249274
_startedTcs.TrySetException(exception);

0 commit comments

Comments
 (0)