From 6ce7496b1e650c366ee3f5c9923c1b03ad84b200 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 18 Nov 2020 10:41:11 -0800 Subject: [PATCH 1/7] Close accept loop when closing connection --- .../Implementations/MsQuic/MsQuicConnection.cs | 3 +++ .../tests/FunctionalTests/QuicConnectionTests.cs | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) 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 fcc975f021dd2..bdf6ac55ecdca 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 @@ -312,6 +312,9 @@ private ValueTask ShutdownAsync( Debug.Assert(_shutdownTcs.Task.IsCompleted == false); + // Stop accepting new streams after calling shutdown. + _acceptQueue.Writer.Complete(); + return new ValueTask(_shutdownTcs.Task); } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index ab4a96132e70d..50ba2d8e37d7d 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -59,6 +59,22 @@ await RunClientServer( Assert.Equal(ExpectedErrorCode, ex.ErrorCode); }); } + + [Fact] + public async Task CloseAsync_ByServer_AcceptThrows() + { + await RunClientServer( + clientConnection => + { + }, + async serverConnection => + { + var acceptTask = serverConnection.AcceptAsync(); + await serverConnection.CloseAsync(); + // make sure + await Assert.ThrowsAsync(() => acceptTask.AsTask()); + }); + } } public sealed class QuicConnectionTests_MockProvider : QuicConnectionTests { } From 4dfff7a68aeb8306ffd8cc388afd7a64f4fc68b1 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 18 Nov 2020 15:44:27 -0800 Subject: [PATCH 2/7] nits --- .../tests/FunctionalTests/QuicConnectionTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index 50ba2d8e37d7d..ca34ca1570b10 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -66,11 +66,12 @@ public async Task CloseAsync_ByServer_AcceptThrows() await RunClientServer( clientConnection => { + return Task.CompletedTask; }, async serverConnection => { - var acceptTask = serverConnection.AcceptAsync(); - await serverConnection.CloseAsync(); + var acceptTask = serverConnection.AcceptStreamAsync(); + await serverConnection.CloseAsync(errorCode: 0); // make sure await Assert.ThrowsAsync(() => acceptTask.AsTask()); }); From 4a1410e0c3093584ce68f8dcdec9139807e5a5a7 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 24 Nov 2020 12:15:18 -0800 Subject: [PATCH 3/7] Trying to debug issues --- .../Implementations/MsQuic/MsQuicConnection.cs | 13 ++++--------- .../Implementations/MsQuic/MsQuicListener.cs | 13 ++++++++++--- .../Quic/Implementations/MsQuic/MsQuicStream.cs | 4 ++-- .../Net/Quic/QuicConnectionAbortedException.cs | 2 +- .../Net/Quic/QuicOperationAbortedException.cs | 2 +- .../Net/Quic/QuicStreamAbortedException.cs | 2 +- .../tests/FunctionalTests/MsQuicTests.cs | 17 +++++++++++++++++ .../FunctionalTests/QuicConnectionTests.cs | 17 ----------------- .../tests/FunctionalTests/QuicListenerTests.cs | 5 +++-- 9 files changed, 39 insertions(+), 36 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 bdf6ac55ecdca..ee5dba0ef41b4 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 @@ -161,21 +161,21 @@ private uint HandleEventShutdownInitiatedByTransport(ref ConnectionEvent connect _connectTcs.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); } - _acceptQueue.Writer.Complete(); - return MsQuicStatusCodes.Success; } private uint HandleEventShutdownInitiatedByPeer(ref ConnectionEvent connectionEvent) { _abortErrorCode = connectionEvent.Data.ShutdownInitiatedByPeer.ErrorCode; - _acceptQueue.Writer.Complete(); return MsQuicStatusCodes.Success; } private uint HandleEventShutdownComplete(ref ConnectionEvent connectionEvent) { _shutdownTcs.SetResult(MsQuicStatusCodes.Success); + + // Stop accepting new streams. + _acceptQueue?.Writer.Complete(); return MsQuicStatusCodes.Success; } @@ -291,7 +291,7 @@ private MsQuicStream StreamOpen( private void SetCallbackHandler() { - Debug.Assert(!_handle.IsAllocated); + Debug.Assert(!_handle.IsAllocated, "callback handler allocated already"); _handle = GCHandle.Alloc(this); MsQuicApi.Api.SetCallbackHandlerDelegate( @@ -310,11 +310,6 @@ private ValueTask ShutdownAsync( ErrorCode); QuicExceptionHelpers.ThrowIfFailed(status, "Failed to shutdown connection."); - Debug.Assert(_shutdownTcs.Task.IsCompleted == false); - - // Stop accepting new streams after calling shutdown. - _acceptQueue.Writer.Complete(); - return new ValueTask(_shutdownTcs.Task); } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs index 60b17a6ad0e01..58bd9fbf0fb9a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs @@ -34,7 +34,7 @@ internal sealed class MsQuicListener : QuicListenerProvider, IDisposable private QuicListenerOptions _options; private volatile bool _disposed; private IPEndPoint _listenEndPoint; - + private bool _started; private readonly Channel _acceptConnectionQueue; internal MsQuicListener(QuicListenerOptions options) @@ -120,6 +120,13 @@ internal override void Start() { ThrowIfDisposed(); + // protect against double dispose here. + if (_started) + { + return; + } + + _started = true; SetCallbackHandler(); SOCKADDR_INET address = MsQuicAddressHelpers.IPEndPointToINet(_listenEndPoint); @@ -186,7 +193,7 @@ internal unsafe uint ListenerCallbackHandler(ref ListenerEvent evt) private void StopAcceptingConnections() { - _acceptConnectionQueue.Writer.TryComplete(); + _acceptConnectionQueue?.Writer.TryComplete(); } private static uint NativeCallbackHandler( @@ -202,7 +209,7 @@ private static uint NativeCallbackHandler( internal void SetCallbackHandler() { - Debug.Assert(!_handle.IsAllocated); + Debug.Assert(!_handle.IsAllocated, "listener allocated"); _handle = GCHandle.Alloc(this); MsQuicApi.Api.SetCallbackHandlerDelegate( diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs index 5379f1ff1ce00..66ea94d218b4d 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs @@ -71,7 +71,7 @@ internal sealed class MsQuicStream : QuicStreamProvider // Creates a new MsQuicStream internal MsQuicStream(MsQuicConnection connection, QUIC_STREAM_OPEN_FLAG flags, IntPtr nativeObjPtr, bool inbound) { - Debug.Assert(connection != null); + Debug.Assert(connection != null, "Connection null"); _ptr = nativeObjPtr; @@ -936,7 +936,7 @@ private unsafe ValueTask SendReadOnlyMemoryListAsync( /// private void StartLocalStream() { - Debug.Assert(!_started); + Debug.Assert(!_started, "start local stream"); uint status = MsQuicApi.Api.StreamStartDelegate( _ptr, (uint)QUIC_STREAM_START_FLAG.ASYNC); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs index ed65d8c06fbb7..cdd2f76bd79e7 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs @@ -6,7 +6,7 @@ namespace System.Net.Quic public class QuicConnectionAbortedException : QuicException { internal QuicConnectionAbortedException(long errorCode) - : this(SR.Format(SR.net_quic_connectionaborted, errorCode), errorCode) + : this("", errorCode) { } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs index 824fcb1ca297a..bd6756a12a796 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs @@ -6,7 +6,7 @@ namespace System.Net.Quic public class QuicOperationAbortedException : QuicException { internal QuicOperationAbortedException() - : base(SR.net_quic_operationaborted) + : base("") { } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs index 77d9ca356bb1b..f873b51a926bc 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs @@ -6,7 +6,7 @@ namespace System.Net.Quic public class QuicStreamAbortedException : QuicException { internal QuicStreamAbortedException(long errorCode) - : this(SR.Format(SR.net_quic_streamaborted, errorCode), errorCode) + : this("", errorCode) { } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 21388ed370230..2614a7a49d7db 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -177,6 +177,23 @@ public async Task CallDifferentWriteMethodsWorks() Assert.Equal(24, res); } + [Fact] + public async Task CloseAsync_ByServer_AcceptThrows() + { + await RunClientServer( + clientConnection => + { + return Task.CompletedTask; + }, + async serverConnection => + { + var acceptTask = serverConnection.AcceptStreamAsync(); + await serverConnection.CloseAsync(errorCode: 0); + // make sure + await Assert.ThrowsAsync(() => acceptTask.AsTask()); + }); + } + private static ReadOnlySequence CreateReadOnlySequenceFromBytes(byte[] data) { List segments = new List diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index ca34ca1570b10..ab4a96132e70d 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -59,23 +59,6 @@ await RunClientServer( Assert.Equal(ExpectedErrorCode, ex.ErrorCode); }); } - - [Fact] - public async Task CloseAsync_ByServer_AcceptThrows() - { - await RunClientServer( - clientConnection => - { - return Task.CompletedTask; - }, - async serverConnection => - { - var acceptTask = serverConnection.AcceptStreamAsync(); - await serverConnection.CloseAsync(errorCode: 0); - // make sure - await Assert.ThrowsAsync(() => acceptTask.AsTask()); - }); - } } public sealed class QuicConnectionTests_MockProvider : QuicConnectionTests { } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs index a1e9bf3b72bae..3fbfcaf75c653 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs @@ -21,10 +21,11 @@ await Task.Run(async () => using QuicListener listener = CreateQuicListener(); using QuicConnection clientConnection = CreateQuicConnection(listener.ListenEndPoint); - await clientConnection.ConnectAsync(); + var clientStreamTask = clientConnection.ConnectAsync(); using QuicConnection serverConnection = await listener.AcceptConnectionAsync(); - }).TimeoutAfter(millisecondsTimeout: 5_000); + await clientStreamTask; + }).TimeoutAfter(millisecondsTimeout: 6_000); } } From f0fa27f72c755dd6b4b233c7ff1ad9e258da56a3 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 24 Nov 2020 13:33:26 -0800 Subject: [PATCH 4/7] nit --- .../System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs | 2 +- .../System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs index 58bd9fbf0fb9a..c4bc455856f76 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs @@ -120,7 +120,7 @@ internal override void Start() { ThrowIfDisposed(); - // protect against double dispose here. + // protect against double starts. if (_started) { return; diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs index e339ac1a30ac6..3da7e7e9bf8d3 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs @@ -305,8 +305,6 @@ public async Task LargeDataSentAndReceived() } } - - [Fact] public async Task TestStreams() { From e71186b685189fc526fb139a227ff0b7798784fa Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Tue, 24 Nov 2020 13:47:28 -0800 Subject: [PATCH 5/7] Restore messages --- .../src/System/Net/Quic/QuicConnectionAbortedException.cs | 2 +- .../src/System/Net/Quic/QuicOperationAbortedException.cs | 2 +- .../src/System/Net/Quic/QuicStreamAbortedException.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs index cdd2f76bd79e7..ed65d8c06fbb7 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionAbortedException.cs @@ -6,7 +6,7 @@ namespace System.Net.Quic public class QuicConnectionAbortedException : QuicException { internal QuicConnectionAbortedException(long errorCode) - : this("", errorCode) + : this(SR.Format(SR.net_quic_connectionaborted, errorCode), errorCode) { } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs index bd6756a12a796..824fcb1ca297a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicOperationAbortedException.cs @@ -6,7 +6,7 @@ namespace System.Net.Quic public class QuicOperationAbortedException : QuicException { internal QuicOperationAbortedException() - : base("") + : base(SR.net_quic_operationaborted) { } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs index f873b51a926bc..77d9ca356bb1b 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStreamAbortedException.cs @@ -6,7 +6,7 @@ namespace System.Net.Quic public class QuicStreamAbortedException : QuicException { internal QuicStreamAbortedException(long errorCode) - : this("", errorCode) + : this(SR.Format(SR.net_quic_streamaborted, errorCode), errorCode) { } From e925068a26b018ad36dce6b0a540d8491e176b71 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 25 Nov 2020 14:54:49 -0800 Subject: [PATCH 6/7] Throw instead of return with double start --- .../System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs index c4bc455856f76..0a1924e877047 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs @@ -123,7 +123,7 @@ internal override void Start() // protect against double starts. if (_started) { - return; + throw new QuicException("Cannot start Listener multiple times"); } _started = true; From 0e048b05f5f3e6e73d53e3dbb680fcd8a7020260 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Wed, 25 Nov 2020 15:59:10 -0800 Subject: [PATCH 7/7] Update src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs Co-authored-by: Stephen Halter --- .../System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs index 0a1924e877047..192085132b963 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs @@ -193,7 +193,7 @@ internal unsafe uint ListenerCallbackHandler(ref ListenerEvent evt) private void StopAcceptingConnections() { - _acceptConnectionQueue?.Writer.TryComplete(); + _acceptConnectionQueue.Writer.TryComplete(); } private static uint NativeCallbackHandler(