Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix GCHandle free for connection and related fixes and asserts #55303

Merged
merged 1 commit into from
Jul 8, 2021
Merged

fix GCHandle free for connection and related fixes and asserts #55303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
View file
Open in desktop
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we either free it in constructor on failure or we expect HandleEventShutdownComplete to always happen, right?

Copy link
Contributor Author

@geoffkizer geoffkizer Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We either (a) fail to register for events somehow (this can happen if either Open or Start fails), in which case we free it immediately and we'll never get any events.

Or (b) we expect to get the SHUTDOWN_COMPLETE event eventually, and we free it then.


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
Open in desktop
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
Open in desktop
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