Skip to content

Commit

Permalink
Fix data race leading to a deadlock.
Browse files Browse the repository at this point in the history
  • Loading branch information
rzikm committed Apr 18, 2024
1 parent 9068070 commit 0ade6f8
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<QuicException>(ex);
Assert.Equal(QuicError.ConnectionAborted, qe.QuicError);
}
}
await connection.HandleRequestAsync();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ public async ValueTask<QuicStream> OpenOutboundStreamAsync(QuicStreamType type,
stream = new QuicStream(_handle, type, _defaultStreamErrorCode);
await stream.StartAsync(cancellationToken).ConfigureAwait(false);
}
catch
catch (Exception ex)
{
if (stream is not null)
{
Expand All @@ -422,8 +422,15 @@ public async ValueTask<QuicStream> 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);
}
Expand Down
31 changes: 28 additions & 3 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 0ade6f8

Please sign in to comment.