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.

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

keep MsQuicConnection alive when streams are pending #52800

Merged
merged 9 commits into from
Jun 10, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ internal sealed class State
public bool Connected;
public long AbortErrorCode = -1;
public int StreamCount;
wfurt marked this conversation as resolved.
Show resolved Hide resolved
public bool Closing;
private IntPtr _closingGCHandle;

// Queue for accepted streams.
// Backlog limit is managed by MsQuic so it can be unbounded here.
Expand All @@ -66,23 +66,34 @@ internal sealed class State

public void RemoveStream(MsQuicStream stream)
{
bool closing;
lock (this)
{
StreamCount--;
wfurt marked this conversation as resolved.
Show resolved Hide resolved
closing = _closingGCHandle != IntPtr.Zero && StreamCount == 0;
}

if (Closing && StreamCount == 0)
if (closing)
{
Handle?.Dispose();
//if (_stateHandle.IsAllocated) _stateHandle.Free();
try
{
GCHandle gcHandle = GCHandle.FromIntPtr(_closingGCHandle);
wfurt marked this conversation as resolved.
Show resolved Hide resolved
Debug.Assert(gcHandle.IsAllocated);
if (gcHandle.IsAllocated) gcHandle.Free();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (gcHandle.IsAllocated) gcHandle.Free();
gcHandle.Free();

Copy link
Member Author

Choose a reason for hiding this comment

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

is this generally safe? We have other places with identical check.

Copy link
Member

Choose a reason for hiding this comment

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

IsAllocated just checks whether the GCHandle is not IntPtr.Zero. _closingGCHandle != IntPtr.Zero above does that already, so it should not be needed here.

}
catch (Exception ex)
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, $"Failed to restore GCHandle from ptr: {ex.Message}");
}
}
}

public bool TryAddStream(SafeMsQuicStreamHandle streamHandle, QUIC_STREAM_OPEN_FLAGS flags)
{
lock (this)
{
if (Closing)
if (_closingGCHandle != IntPtr.Zero)
{
return false;
}
Expand All @@ -101,6 +112,12 @@ public bool TryAddStream(SafeMsQuicStreamHandle streamHandle, QUIC_STREAM_OPEN_F
}
}
}

// This is called under lock from connection dispose
public void SetClosing(GCHandle handle)
{
_closingGCHandle = GCHandle.ToIntPtr(handle);
}
}

// constructor for inbound connections
Expand Down Expand Up @@ -568,15 +585,23 @@ private void Dispose(bool disposing)
lock (_state)
{
_state.Connection = null;
_state.Closing = true;
if (_state.StreamCount == 0)
{
_state?.Handle?.Dispose();
_state!.Handle?.Dispose();
if (_stateHandle.IsAllocated) _stateHandle.Free();
}
else
{
// normally, _state would be rooted by 'this' and we would hold _stateHandle to prevent
// GC from moving _state as we handed pointer to it by msquic. It was handed to msquic and
// we may try to get _state from it in NativeCallbackHandler()
// At this point we are being disposed either explicitly or by finalizer but we still have active streams.
// To prevent issue, the handle will be transferred to state and it will be released when last stream is gone
_state.SetClosing(_stateHandle);
}
}

FlushAcceptQueue().GetAwaiter().GetResult();
Copy link
Contributor

Choose a reason for hiding this comment

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

Connections are IAsyncDisposable; we should make dispose do proper await there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you suggest. We still need to flush for synchronous Dispose(). Can we do what ever needs to be done as follow-up? I would like to get this this in to get verifications on the crashes.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine.

if (_stateHandle.IsAllocated) _stateHandle.Free();
}

// TODO: this appears abortive and will cause prior successfully shutdown and closed streams to drop data.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@ private void Dispose(bool disposing)
Marshal.FreeHGlobal(_state.SendQuicBuffers);
if (_stateHandle.IsAllocated) _stateHandle.Free();
CleanupSendState(_state);
Debug.Assert(_state.ConnectionState != null);
_state.ConnectionState?.RemoveStream(this);

if (NetEventSource.Log.IsEnabled())
Expand Down