Skip to content

Commit

Permalink
fix GCHandle free for connection and related fixes and asserts (#55303)
Browse files Browse the repository at this point in the history 
Co-authored-by: Geoffrey Kizer <geoffrek@windows.microsoft.com>
  • Loading branch information
@geoffkizer
geoffkizer and Geoffrey Kizer authored Jul 8, 2021
1 parent b2bc284 commit b877bcd
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 10 deletions.
21 changes: 15 additions & 6 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ public void RemoveStream(MsQuicStream? stream)
if (releaseHandles)
{
Handle?.Dispose();
StateGCHandle.Free();
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
{
Expand All @@ -547,6 +551,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d
}
catch
{
_state.StateGCHandle.Free();
_state.Connection = null;
throw;
}
Expand Down Expand Up @@ -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())
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
}

Expand Down
30 changes: 28 additions & 2 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicListener.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -59,7 +60,6 @@ internal MsQuicListener(QuicListenerOptions options)
}
catch
{
_state.Handle?.Dispose();
_stateHandle.Free();
throw;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -123,13 +131,20 @@ private unsafe IPEndPoint Start(QuicListenerOptions options)

uint status;

Debug.Assert(_stateHandle.IsAllocated);

MemoryHandle[]? handles = null;
QuicBuffer[]? buffers = null;
try
{
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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 10 additions & 2 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
}
}
Expand Down

0 comments on commit b877bcd

Please sign in to comment.