-
Notifications
You must be signed in to change notification settings - Fork 5.4k
close quic streams on Dispose #54798
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
Changes from all commits
7af33bf
3597c1f
24adc3f
53d7f5b
26f9d4b
7c2b71a
f107c95
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like there are a bunch of places now where we either call Writer.Complete or Writer.TryComplete. Are all of these necessary? I would assume we only have to do this in a limited number of places, e.g. HandleEventShutdownInitiatedByTransport and HandleEventShutdownInitiatedByPeer, and maybe one other for when we initiate shutdown ourselves. I always get nervous when we do stuff like call TryComplete instead of Complete, because it seems like we don't have a clean handling of exactly when the Complete operation needs to happen. I'd prefer to see the TryComplete calls changed to either Complete or to assert that it's already completed.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case I was getting exception when when the connection was already Disposed and that triggered flush of events and ShutdownComplete. I could wrap it with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I look at the cases @geoffkizer and it is not that we would not know if the Complete operation should happen. We just don't know if happened before. Like in this case the shutdown may be initiated by peer or somebody may Dispose the connection but than we get this final event and it will throw if we try to complete it again. Unless I missed something I did not find API on the Writer to know if it was completed or not.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine for this PR, but we should revisit the logic here moving forward. |
||
|
|
||
| // 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(); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we even passing in the stream? Doesn't seem to be used at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going back and forth on this one. When debugging it is very nice to have the stream here so one can emit traces from single location. This is basically same for the AddStream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand why we need this at all... It seems like we are deferring Dispose of the connection until all streams in use on it are completed. Is that correct? Why do we want to do this? Can we just get rid of this tracking completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that is the intend. We saw crashes without it but it can perhaps be done in different way. If so I would do it in separate PR. The reason why this was touches is that the HandleEventShutdownComplete does not have access to the Stream itself so it would pass NULL.
I'm OK removing the parameter completely as it is not use for product or perhaps using it Debug builds only and for example dump the Stream info if we are on path to Assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should consider for a separate PR.
Mostly I want to make sure we are making an intentional decision about behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When chatting with @nibanks he sad we would not get HandleEventShutdownComplete with pending streams. So if we delay cleanup in Dispose and if we can relay on that we could perhaps remove to logic. It would be nice to have stress tests up at that time so we can verify the impact.