close quic streams on Dispose (dotnet#54798)

* close quic streams on Dispose * feedback from review * update ShutdownState handling * feedback from review * fix failing cancelling test Co-authored-by: Marie Píchová <[email protected]>
NinoFloris · Jul 7, 2021 · 0d1e9ca · 0d1e9ca
1 parent 1c79e60
commit 0d1e9ca
Show file tree

Hide file tree

Showing 10 changed files with 159 additions and 64 deletions.
diff --git a/....Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs b/....Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs
@@ -28,6 +28,7 @@ private SafeMsQuicConfigurationHandle()
         protected override bool ReleaseHandle()
         {
             MsQuicApi.Api.ConfigurationCloseDelegate(handle);
+            SetHandle(IntPtr.Zero);
             return true;
         }
 

diff --git a/...Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConnectionHandle.cs b/...Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConnectionHandle.cs
@@ -22,6 +22,7 @@ public SafeMsQuicConnectionHandle(IntPtr connectionHandle)
         protected override bool ReleaseHandle()
         {
             MsQuicApi.Api.ConnectionCloseDelegate(handle);
+            SetHandle(IntPtr.Zero);
             return true;
         }
     }

diff --git a/...m.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicListenerHandle.cs b/...m.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicListenerHandle.cs
@@ -16,6 +16,7 @@ private SafeMsQuicListenerHandle()
         protected override bool ReleaseHandle()
         {
             MsQuicApi.Api.ListenerCloseDelegate(handle);
+            SetHandle(IntPtr.Zero);
             return true;
         }
     }

diff --git a/...t.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicRegistrationHandle.cs b/...t.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicRegistrationHandle.cs
@@ -16,6 +16,7 @@ private SafeMsQuicRegistrationHandle()
         protected override bool ReleaseHandle()
         {
             MsQuicApi.Api.RegistrationCloseDelegate(handle);
+            SetHandle(IntPtr.Zero);
             return true;
         }
     }

diff --git a/...tem.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicStreamHandle.cs b/...tem.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicStreamHandle.cs
@@ -22,6 +22,7 @@ public SafeMsQuicStreamHandle(IntPtr streamHandle)
         protected override bool ReleaseHandle()
         {
             MsQuicApi.Api.StreamCloseDelegate(handle);
+            SetHandle(IntPtr.Zero);
             return true;
         }
     }

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
@@ -20,6 +20,7 @@ internal sealed class MsQuicConnection : QuicConnectionProvider
     {
         private static readonly Oid s_clientAuthOid = new Oid("1.3.6.1.5.5.7.3.2", "1.3.6.1.5.5.7.3.2");
         private static readonly Oid s_serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1", "1.3.6.1.5.5.7.3.1");
+        private const uint DefaultResetValue = 0xffffffff; // Arbitrary value unlikely to conflict with application protocols.
 
         // Delegate that wraps the static function that will be called when receiving an event.
         private static readonly ConnectionCallbackDelegate s_connectionDelegate = new ConnectionCallbackDelegate(NativeCallbackHandler);
@@ -70,7 +71,7 @@ internal sealed class State
                 SingleWriter = true,
             });
 
-            public void RemoveStream(MsQuicStream stream)
+            public void RemoveStream(MsQuicStream? stream)
             {
                 bool releaseHandles;
                 lock (this)
@@ -252,7 +253,7 @@ private static uint HandleEventShutdownComplete(State state, ref ConnectionEvent
             state.ShutdownTcs.SetResult(MsQuicStatusCodes.Success);
 
             // Stop accepting new streams.
-            state.AcceptQueue.Writer.Complete();
+            state.AcceptQueue.Writer.TryComplete();
 
             // Stop notifying about available streams.
             TaskCompletionSource? unidirectionalTcs = null;
@@ -625,7 +626,7 @@ private static uint NativeCallbackHandler(
             {
                 if (NetEventSource.Log.IsEnabled())
                 {
-                    NetEventSource.Error(state, $"Exception occurred during connection callback: {ex.Message}");
+                    NetEventSource.Error(state, $"[Connection#{state.GetHashCode()}] Exception occurred during handling {connectionEvent.Type} connection callback: {ex.Message}");
                 }
 
                 // TODO: trigger an exception on any outstanding async calls.
@@ -648,9 +649,17 @@ public override void Dispose()
         private async Task FlushAcceptQueue()
         {
             _state.AcceptQueue.Writer.TryComplete();
-            await foreach (MsQuicStream item in _state.AcceptQueue.Reader.ReadAllAsync().ConfigureAwait(false))
+            await foreach (MsQuicStream stream in _state.AcceptQueue.Reader.ReadAllAsync().ConfigureAwait(false))
             {
-                item.Dispose();
+                if (stream.CanRead)
+                {
+                    stream.AbortRead(DefaultResetValue);
+                }
+                if (stream.CanWrite)
+                {
+                    stream.AbortWrite(DefaultResetValue);
+                }
+                stream.Dispose();
             }
         }
 
@@ -681,7 +690,8 @@ private void Dispose(bool disposing)
             _configuration?.Dispose();
             if (releaseHandles)
             {
-                _state!.Handle?.Dispose();
+                // We may not be fully initialized if constructor fails.
+                _state.Handle?.Dispose();
                 if (_state.StateGCHandle.IsAllocated) _state.StateGCHandle.Free();
             }
         }