From 6e8b357c143dbce58ce92421b85a4d77d54c1645 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Sun, 11 Jul 2021 21:26:13 -0700 Subject: [PATCH 1/3] do abortive connection shutdown in MsQuicConnection.Dispose, add test for CloseAsync and Dispose, and remove CloseAsync from RunClientServer as it should not be necessary anymore --- .../MsQuic/MsQuicConnection.cs | 6 + .../FunctionalTests/QuicConnectionTests.cs | 147 +++++++++++++++++- .../tests/FunctionalTests/QuicTestBase.cs | 2 - 3 files changed, 151 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs index c5df9a21b468b1..137ddea67992f8 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs @@ -707,6 +707,12 @@ private void Dispose(bool disposing) if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(_state, $"{TraceId()} disposing {disposing}"); + // If we haven't already shutdown gracefully (via a successful CloseAsync call), then force an abortive shutdown. + MsQuicApi.Api.ConnectionShutdownDelegate( + _state.Handle, + QUIC_CONNECTION_SHUTDOWN_FLAGS.SILENT, + 0); + bool releaseHandles = false; lock (_state) { diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index 41b8d549ed7480..bce2e073659798 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -11,6 +11,8 @@ namespace System.Net.Quic.Tests public abstract class QuicConnectionTests : QuicTestBase where T : IQuicImplProviderFactory, new() { + const int ExpectedErrorCode = 1234; + [Fact] public async Task TestConnect() { @@ -39,8 +41,6 @@ public async Task TestConnect() [ActiveIssue("https://github.com/dotnet/runtime/issues/55242", TestPlatforms.Linux)] public async Task AcceptStream_ConnectionAborted_ByClient_Throws() { - const int ExpectedErrorCode = 1234; - using var sync = new SemaphoreSlim(0); await RunClientServer( @@ -56,6 +56,149 @@ await RunClientServer( Assert.Equal(ExpectedErrorCode, ex.ErrorCode); }); } + + private static async Task DoWrites(QuicStream writer, int writeCount) + { + for (int i = 0; i < writeCount; i++) + { + await writer.WriteAsync(new byte[1]); + } + } + + private static async Task DoReads(QuicStream reader, int readCount) + { + for (int i = 0; i < readCount; i++) + { + int bytesRead = await reader.ReadAsync(new byte[1]); + Assert.Equal(1, bytesRead); + } + } + + [Theory] + // [InlineData(0)] // Fails with TimeoutException -- needs investigation + [InlineData(1)] + [InlineData(10)] + public async Task CloseAsync_WithOpenLocalStream_LocalStreamFailsWithQuicOperationAbortedException(int writesBeforeClose) + { + if (typeof(T) == typeof(MockProviderFactory)) + { + return; + } + + using var sync = new SemaphoreSlim(0); + + await RunClientServer( + async clientConnection => + { + using QuicStream clientStream = clientConnection.OpenBidirectionalStream(); + await DoWrites(clientStream, writesBeforeClose); + + // Wait for peer to receive data + await sync.WaitAsync(); + + await clientConnection.CloseAsync(ExpectedErrorCode); + + await Assert.ThrowsAsync(async () => await clientStream.ReadAsync(new byte[1])); + await Assert.ThrowsAsync(async () => await clientStream.WriteAsync(new byte[1])); + + sync.Release(); + }, + async serverConnection => + { + using QuicStream serverStream = await serverConnection.AcceptStreamAsync(); + await DoReads(serverStream, writesBeforeClose); + + sync.Release(); + + // Wait for peer to fail stream + await sync.WaitAsync(); + }); + } + + [Theory] + // [InlineData(0)] // Fails with TimeoutException -- needs investigation + [InlineData(1)] + [InlineData(10)] + public async Task Dispose_WithOpenLocalStream_LocalStreamFailsWithQuicOperationAbortedException(int writesBeforeClose) + { + if (typeof(T) == typeof(MockProviderFactory)) + { + return; + } + + using var sync = new SemaphoreSlim(0); + + await RunClientServer( + async clientConnection => + { + using QuicStream clientStream = clientConnection.OpenBidirectionalStream(); + await DoWrites(clientStream, writesBeforeClose); + + // Wait for peer to receive data + await sync.WaitAsync(); + + clientConnection.Dispose(); + + await Assert.ThrowsAsync(async () => await clientStream.ReadAsync(new byte[1])); + await Assert.ThrowsAsync(async () => await clientStream.WriteAsync(new byte[1])); + + sync.Release(); + }, + async serverConnection => + { + using QuicStream serverStream = await serverConnection.AcceptStreamAsync(); + await DoReads(serverStream, writesBeforeClose); + + sync.Release(); + + // Wait for peer to fail stream + await sync.WaitAsync(); + }); + } + + [Theory] + // [InlineData(0)] // Fails with TimeoutException -- needs investigation + [InlineData(1)] + [InlineData(10)] + public async Task CloseAsync_WithOpenPeerStream_PeerStreamFailsWithQuicConnectionAbortedException(int writesBeforeClose) + { + if (typeof(T) == typeof(MockProviderFactory)) + { + return; + } + + using var sync = new SemaphoreSlim(0); + + await RunClientServer( + async clientConnection => + { + using QuicStream clientStream = clientConnection.OpenBidirectionalStream(); + await DoWrites(clientStream, writesBeforeClose); + + // Wait for peer to receive data + await sync.WaitAsync(); + + QuicConnectionAbortedException ex; + ex = await Assert.ThrowsAsync(async () => await clientStream.ReadAsync(new byte[1])); + Assert.Equal(ExpectedErrorCode, ex.ErrorCode); + ex = await Assert.ThrowsAsync(async () => await clientStream.WriteAsync(new byte[1])); + Assert.Equal(ExpectedErrorCode, ex.ErrorCode); + + sync.Release(); + }, + async serverConnection => + { + using QuicStream serverStream = await serverConnection.AcceptStreamAsync(); + await DoReads(serverStream, writesBeforeClose); + + sync.Release(); + + await serverConnection.CloseAsync(ExpectedErrorCode); + + // Wait for peer to fail stream + await sync.WaitAsync(); + }); + } } public sealed class QuicConnectionTests_MockProvider : QuicConnectionTests { } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs index 4d9ab4de6a3aa2..fed05f0a55bd70 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs @@ -130,7 +130,6 @@ await new[] await serverFunction(serverConnection); serverFinished.Set(); clientFinished.Wait(); - await serverConnection.CloseAsync(0); }), Task.Run(async () => { @@ -139,7 +138,6 @@ await new[] await clientFunction(clientConnection); clientFinished.Set(); serverFinished.Wait(); - await clientConnection.CloseAsync(0); }) }.WhenAllOrAnyFailed(millisecondsTimeout); } From 3994cfec14405f92de4a0e0b52083914af634ef9 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Mon, 12 Jul 2021 20:56:35 -0700 Subject: [PATCH 2/3] CloseAsync is no-op if disposed --- .../Net/Quic/Implementations/MsQuic/MsQuicConnection.cs | 5 ++++- .../System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs index 137ddea67992f8..0df70b889e058c 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs @@ -743,7 +743,10 @@ private void Dispose(bool disposing) // It's unclear how to gracefully wait for a connection to be 100% done. internal override ValueTask CloseAsync(long errorCode, CancellationToken cancellationToken = default) { - ThrowIfDisposed(); + if (_disposed == 1) + { + return default; + } return ShutdownAsync(QUIC_CONNECTION_SHUTDOWN_FLAGS.NONE, errorCode); } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs index fed05f0a55bd70..e58734c0d9253e 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs @@ -112,6 +112,9 @@ internal async Task PingPong(QuicConnection client, QuicConnection server) internal async Task RunClientServer(Func clientFunction, Func serverFunction, int iterations = 1, int millisecondsTimeout = 10_000) { + const long ClientCloseErrorCode = 11111; + const long ServerCloseErrorCode = 22222; + using QuicListener listener = CreateQuicListener(); var serverFinished = new ManualResetEventSlim(); @@ -130,6 +133,7 @@ await new[] await serverFunction(serverConnection); serverFinished.Set(); clientFinished.Wait(); + await serverConnection.CloseAsync(ServerCloseErrorCode); }), Task.Run(async () => { @@ -138,6 +142,7 @@ await new[] await clientFunction(clientConnection); clientFinished.Set(); serverFinished.Wait(); + await clientConnection.CloseAsync(ClientCloseErrorCode); }) }.WhenAllOrAnyFailed(millisecondsTimeout); } From 81e626f3f258c2965144042d3e730094416f440f Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Mon, 12 Jul 2021 21:20:40 -0700 Subject: [PATCH 3/3] more review feedback --- .../FunctionalTests/QuicConnectionTests.cs | 73 +++++-------------- .../tests/FunctionalTests/QuicTestBase.cs | 20 +++-- 2 files changed, 30 insertions(+), 63 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index bce2e073659798..9988961e81e5af 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -75,10 +75,9 @@ private static async Task DoReads(QuicStream reader, int readCount) } [Theory] - // [InlineData(0)] // Fails with TimeoutException -- needs investigation [InlineData(1)] [InlineData(10)] - public async Task CloseAsync_WithOpenLocalStream_LocalStreamFailsWithQuicOperationAbortedException(int writesBeforeClose) + public async Task CloseAsync_WithOpenStream_LocalAndPeerStreamsFailWithQuicOperationAbortedException(int writesBeforeClose) { if (typeof(T) == typeof(MockProviderFactory)) { @@ -100,8 +99,6 @@ await RunClientServer( await Assert.ThrowsAsync(async () => await clientStream.ReadAsync(new byte[1])); await Assert.ThrowsAsync(async () => await clientStream.WriteAsync(new byte[1])); - - sync.Release(); }, async serverConnection => { @@ -110,13 +107,17 @@ await RunClientServer( sync.Release(); - // Wait for peer to fail stream - await sync.WaitAsync(); + // Since the peer did the abort, we should receive the abort error code in the exception. + QuicConnectionAbortedException ex; + ex = await Assert.ThrowsAsync(async () => await serverStream.ReadAsync(new byte[1])); + Assert.Equal(ExpectedErrorCode, ex.ErrorCode); + ex = await Assert.ThrowsAsync(async () => await serverStream.WriteAsync(new byte[1])); + Assert.Equal(ExpectedErrorCode, ex.ErrorCode); }); } + [OuterLoop("Depends on IdleTimeout")] [Theory] - // [InlineData(0)] // Fails with TimeoutException -- needs investigation [InlineData(1)] [InlineData(10)] public async Task Dispose_WithOpenLocalStream_LocalStreamFailsWithQuicOperationAbortedException(int writesBeforeClose) @@ -126,6 +127,10 @@ public async Task Dispose_WithOpenLocalStream_LocalStreamFailsWithQuicOperationA return; } + // Set a short idle timeout so that after we dispose the connection, the peer will discover the connection is dead before too long. + QuicListenerOptions listenerOptions = CreateQuicListenerOptions(); + listenerOptions.IdleTimeout = TimeSpan.FromSeconds(1); + using var sync = new SemaphoreSlim(0); await RunClientServer( @@ -141,8 +146,6 @@ await RunClientServer( await Assert.ThrowsAsync(async () => await clientStream.ReadAsync(new byte[1])); await Assert.ThrowsAsync(async () => await clientStream.WriteAsync(new byte[1])); - - sync.Release(); }, async serverConnection => { @@ -151,53 +154,11 @@ await RunClientServer( sync.Release(); - // Wait for peer to fail stream - await sync.WaitAsync(); - }); - } - - [Theory] - // [InlineData(0)] // Fails with TimeoutException -- needs investigation - [InlineData(1)] - [InlineData(10)] - public async Task CloseAsync_WithOpenPeerStream_PeerStreamFailsWithQuicConnectionAbortedException(int writesBeforeClose) - { - if (typeof(T) == typeof(MockProviderFactory)) - { - return; - } - - using var sync = new SemaphoreSlim(0); - - await RunClientServer( - async clientConnection => - { - using QuicStream clientStream = clientConnection.OpenBidirectionalStream(); - await DoWrites(clientStream, writesBeforeClose); - - // Wait for peer to receive data - await sync.WaitAsync(); - - QuicConnectionAbortedException ex; - ex = await Assert.ThrowsAsync(async () => await clientStream.ReadAsync(new byte[1])); - Assert.Equal(ExpectedErrorCode, ex.ErrorCode); - ex = await Assert.ThrowsAsync(async () => await clientStream.WriteAsync(new byte[1])); - Assert.Equal(ExpectedErrorCode, ex.ErrorCode); - - sync.Release(); - }, - async serverConnection => - { - using QuicStream serverStream = await serverConnection.AcceptStreamAsync(); - await DoReads(serverStream, writesBeforeClose); - - sync.Release(); - - await serverConnection.CloseAsync(ExpectedErrorCode); - - // Wait for peer to fail stream - await sync.WaitAsync(); - }); + // The client has done an abortive shutdown of the connection, which means we are not notified that the connection has closed. + // But the connection idle timeout should kick in and eventually we will get exceptions. + await Assert.ThrowsAsync(async () => await serverStream.ReadAsync(new byte[1])); + await Assert.ThrowsAsync(async () => await serverStream.WriteAsync(new byte[1])); + }, listenerOptions: listenerOptions); } } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs index e58734c0d9253e..c538730a2616aa 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs @@ -56,15 +56,21 @@ internal QuicConnection CreateQuicConnection(IPEndPoint endpoint) return new QuicConnection(ImplementationProvider, endpoint, GetSslClientAuthenticationOptions()); } - internal QuicListener CreateQuicListener(int maxUnidirectionalStreams = 100, int maxBidirectionalStreams = 100) + internal QuicListenerOptions CreateQuicListenerOptions() { - var options = new QuicListenerOptions() + return new QuicListenerOptions() { ListenEndPoint = new IPEndPoint(IPAddress.Loopback, 0), - ServerAuthenticationOptions = GetSslServerAuthenticationOptions(), - MaxUnidirectionalStreams = maxUnidirectionalStreams, - MaxBidirectionalStreams = maxBidirectionalStreams + ServerAuthenticationOptions = GetSslServerAuthenticationOptions() }; + } + + internal QuicListener CreateQuicListener(int maxUnidirectionalStreams = 100, int maxBidirectionalStreams = 100) + { + var options = CreateQuicListenerOptions(); + options.MaxUnidirectionalStreams = maxUnidirectionalStreams; + options.MaxBidirectionalStreams = maxBidirectionalStreams; + return CreateQuicListener(options); } @@ -110,12 +116,12 @@ internal async Task PingPong(QuicConnection client, QuicConnection server) private QuicListener CreateQuicListener(QuicListenerOptions options) => new QuicListener(ImplementationProvider, options); - internal async Task RunClientServer(Func clientFunction, Func serverFunction, int iterations = 1, int millisecondsTimeout = 10_000) + internal async Task RunClientServer(Func clientFunction, Func serverFunction, int iterations = 1, int millisecondsTimeout = 10_000, QuicListenerOptions listenerOptions = null) { const long ClientCloseErrorCode = 11111; const long ServerCloseErrorCode = 22222; - using QuicListener listener = CreateQuicListener(); + using QuicListener listener = CreateQuicListener(listenerOptions ?? CreateQuicListenerOptions()); var serverFinished = new ManualResetEventSlim(); var clientFinished = new ManualResetEventSlim();