Skip to content

Commit

Permalink
Quic Abort after Dispose is no-op (#57318)
Browse files Browse the repository at this point in the history
Allow calling Abort(Read|Write) after disposing. The only safe option to do so is to make them no-op in that case.

Fixes #56683
  • Loading branch information
CarnaViire authored Aug 13, 2021
1 parent 60ee283 commit d67efc5
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,7 @@ public async Task ResponseCancellation_BothCancellationTokenAndDispose_Success()
Exception ex = await Assert.ThrowsAnyAsync<Exception>(() => stream.ReadAsync(new byte[1024], cancellationToken: cts.Token).AsTask());
// exact exception depends on who won the race
if (ex is not OperationCanceledException and not ObjectDisposedException)
if (ex is not OperationCanceledException)
{
var ioe = Assert.IsType<IOException>(ex);
var hre = Assert.IsType<HttpRequestException>(ioe.InnerException);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,11 @@ private static unsafe int CopyMsQuicBuffersToUserBuffer(ReadOnlySpan<QuicBuffer>

internal override void AbortRead(long errorCode)
{
ThrowIfDisposed();
if (_disposed == 1)
{
// Dispose called AbortRead already
return;
}

bool shouldComplete = false;
lock (_state)
Expand Down Expand Up @@ -562,7 +566,13 @@ internal override void AbortRead(long errorCode)

internal override void AbortWrite(long errorCode)
{
ThrowIfDisposed();
if (_disposed == 1)
{
// Dispose already triggered graceful shutdown
// It is unsafe to try to trigger abortive shutdown now, because final event arriving after Dispose releases SafeHandle
// so if it arrives after our check but before we call msquic, me might end up with access violation
return;
}

lock (_state)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,65 @@ async Task ReadUntilAborted()
}
);
}

[Fact]
public async Task AbortAfterDispose_ProperlyOpenedStream_Success()
{
byte[] buffer = new byte[1] { 42 };
var sem = new SemaphoreSlim(0);

await RunClientServer(
clientFunction: async connection =>
{
QuicStream stream = connection.OpenBidirectionalStream();
// Force stream to open on the wire
await stream.WriteAsync(buffer);
await sem.WaitAsync();
stream.Dispose();
// should not throw ODE on aborting
stream.AbortRead(1234);
stream.AbortWrite(5675);
},
serverFunction: async connection =>
{
await using QuicStream stream = await connection.AcceptStreamAsync();
Assert.Equal(1, await stream.ReadAsync(buffer));
sem.Release();
// client will abort both sides, so we will receive the final event
await stream.ShutdownCompleted();
}
);
}

[Fact]
public async Task AbortAfterDispose_StreamCreationFlushedByDispose_Success()
{
await RunClientServer(
clientFunction: connection =>
{
QuicStream stream = connection.OpenBidirectionalStream();
// dispose will flush stream creation on the wire
stream.Dispose();
// should not throw ODE on aborting
stream.AbortRead(1234);
stream.AbortWrite(5675);
return Task.CompletedTask;
},
serverFunction: async connection =>
{
await using QuicStream stream = await connection.AcceptStreamAsync();
// client will abort both sides, so we will receive the final event
await stream.ShutdownCompleted();
}
);
}
}

public sealed class QuicStreamTests_MockProvider : QuicStreamTests<MockProviderFactory>
Expand Down

0 comments on commit d67efc5

Please sign in to comment.