From 44b3c386b5518dfd71e7e2094325645288de45ed Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 7 Jul 2021 13:34:59 -0700 Subject: [PATCH] fix GCHandle free for connection and related fixes and asserts --- .../MsQuic/MsQuicConnection.cs | 21 +++++++++---- .../Implementations/MsQuic/MsQuicListener.cs | 30 +++++++++++++++++-- .../Implementations/MsQuic/MsQuicStream.cs | 12 ++++++-- 3 files changed, 53 insertions(+), 10 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 9e4005a6b5a76..a90f03467b0a0 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 @@ -84,7 +84,6 @@ public void RemoveStream(MsQuicStream? stream) if (releaseHandles) { Handle?.Dispose(); - StateGCHandle.Free(); } } @@ -235,19 +234,22 @@ private static uint HandleEventShutdownInitiatedByTransport(State state, ref Con state.ConnectTcs.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); } - state.AcceptQueue.Writer.Complete(); + state.AcceptQueue.Writer.TryComplete(); return MsQuicStatusCodes.Success; } private static uint HandleEventShutdownInitiatedByPeer(State state, ref ConnectionEvent connectionEvent) { state.AbortErrorCode = (long)connectionEvent.Data.ShutdownInitiatedByPeer.ErrorCode; - state.AcceptQueue.Writer.Complete(); + state.AcceptQueue.Writer.TryComplete(); return MsQuicStatusCodes.Success; } private static uint HandleEventShutdownComplete(State state, ref ConnectionEvent connectionEvent) { + // This is the final event on the connection, so free the GCHandle used by the event callback. + state.StateGCHandle.Free(); + state.Connection = null; state.ShutdownTcs.SetResult(MsQuicStatusCodes.Success); @@ -533,6 +535,8 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d _ => throw new Exception(SR.Format(SR.net_quic_unsupported_address_family, _remoteEndPoint.AddressFamily)) }; + Debug.Assert(_state.StateGCHandle.IsAllocated); + _state.Connection = this; try { @@ -547,6 +551,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d } catch { + _state.StateGCHandle.Free(); _state.Connection = null; throw; } @@ -593,7 +598,10 @@ private static uint NativeCallbackHandler( IntPtr context, ref ConnectionEvent connectionEvent) { - var state = (State)GCHandle.FromIntPtr(context).Target!; + GCHandle gcHandle = GCHandle.FromIntPtr(context); + Debug.Assert(gcHandle.IsAllocated); + Debug.Assert(gcHandle.Target is not null); + var state = (State)gcHandle.Target; if (NetEventSource.Log.IsEnabled()) { @@ -626,9 +634,11 @@ private static uint NativeCallbackHandler( { if (NetEventSource.Log.IsEnabled()) { - NetEventSource.Error(state, $"[Connection#{state.GetHashCode()}] Exception occurred during handling {connectionEvent.Type} connection callback: {ex.Message}"); + NetEventSource.Error(state, $"[Connection#{state.GetHashCode()}] Exception occurred during handling {connectionEvent.Type} connection callback: {ex}"); } + Debug.Fail($"[Connection#{state.GetHashCode()}] Exception occurred during handling {connectionEvent.Type} connection callback: {ex}"); + // TODO: trigger an exception on any outstanding async calls. return MsQuicStatusCodes.InternalError; @@ -692,7 +702,6 @@ private void Dispose(bool disposing) { // We may not be fully initialized if constructor fails. _state.Handle?.Dispose(); - if (_state.StateGCHandle.IsAllocated) _state.StateGCHandle.Free(); } } 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 7d18fa08a479d..97c1973e081c2 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 @@ -3,6 +3,7 @@ using System.Buffers; using System.Collections.Generic; +using System.Diagnostics; using System.Net.Quic.Implementations.MsQuic.Internal; using System.Net.Security; using System.Runtime.InteropServices; @@ -59,7 +60,6 @@ internal MsQuicListener(QuicListenerOptions options) } catch { - _state.Handle?.Dispose(); _stateHandle.Free(); throw; } @@ -109,7 +109,15 @@ private void Dispose(bool disposing) Stop(); _state?.Handle?.Dispose(); + + // Note that it's safe to free the state GCHandle here, because: + // (1) We called ListenerStop above, which will block until all listener events are processed. So we will not receive any more listener events. + // (2) This class is finalizable, which means we will always get called even if the user doesn't explicitly Dispose us. + // If we ever change this class to not be finalizable, and instead rely on the SafeHandle finalization, then we will need to make + // the SafeHandle responsible for freeing this GCHandle, since it will have the only chance to do so when finalized. + if (_stateHandle.IsAllocated) _stateHandle.Free(); + _state?.ConnectionConfiguration?.Dispose(); _disposed = true; } @@ -123,6 +131,8 @@ private unsafe IPEndPoint Start(QuicListenerOptions options) uint status; + Debug.Assert(_stateHandle.IsAllocated); + MemoryHandle[]? handles = null; QuicBuffer[]? buffers = null; try @@ -130,6 +140,11 @@ private unsafe IPEndPoint Start(QuicListenerOptions options) MsQuicAlpnHelper.Prepare(applicationProtocols, out handles, out buffers); status = MsQuicApi.Api.ListenerStartDelegate(_state.Handle, (QuicBuffer*)Marshal.UnsafeAddrOfPinnedArrayElement(buffers, 0), (uint)applicationProtocols.Count, ref address); } + catch + { + _stateHandle.Free(); + throw; + } finally { MsQuicAlpnHelper.Return(ref handles, ref buffers); @@ -167,7 +182,11 @@ private static unsafe uint NativeCallbackHandler( return MsQuicStatusCodes.InternalError; } - State state = (State)GCHandle.FromIntPtr(context).Target!; + GCHandle gcHandle = GCHandle.FromIntPtr(context); + Debug.Assert(gcHandle.IsAllocated); + Debug.Assert(gcHandle.Target is not null); + var state = (State)gcHandle.Target; + SafeMsQuicConnectionHandle? connectionHandle = null; try @@ -197,6 +216,13 @@ private static unsafe uint NativeCallbackHandler( } catch (Exception ex) { + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(state, $"[Listener#{state.GetHashCode()}] Exception occurred during handling {(QUIC_LISTENER_EVENT)evt.Type} connection callback: {ex}"); + } + + Debug.Fail($"[Listener#{state.GetHashCode()}] Exception occurred during handling {(QUIC_LISTENER_EVENT)evt.Type} connection callback: {ex}"); + // This handle will be cleaned up by MsQuic by returning InternalError. connectionHandle?.SetHandleAsInvalid(); state.AcceptConnectionQueue.Writer.TryComplete(ex); 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 1d4d43ef72f6c..a94a7efeb2710 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 @@ -695,7 +695,11 @@ private static uint NativeCallbackHandler( IntPtr context, ref StreamEvent streamEvent) { - var state = (State)GCHandle.FromIntPtr(context).Target!; + GCHandle gcHandle = GCHandle.FromIntPtr(context); + Debug.Assert(gcHandle.IsAllocated); + Debug.Assert(gcHandle.Target is not null); + var state = (State)gcHandle.Target; + return HandleEvent(state, ref streamEvent); } @@ -746,9 +750,13 @@ private static uint HandleEvent(State state, ref StreamEvent evt) { if (NetEventSource.Log.IsEnabled()) { - NetEventSource.Error(state, $"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex.Message}"); + NetEventSource.Error(state, $"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex}"); } + // This is getting hit currently + // See https://github.com/dotnet/runtime/issues/55302 + //Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex}"); + return MsQuicStatusCodes.InternalError; } }