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

Support Delaying Stream ID FC Updates to StreamClose #3665

Merged
merged 22 commits into from
Jun 9, 2023

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Jun 1, 2023

Description

Sometimes apps might want to delay the stream ID FC updates to StreamClose if they intend on doing significant work between SHUTDOWN_COMPLETE and the close call.

Testing

Updated spinquic.

Documentation

Updated.

@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Jun 1, 2023
@nibanks nibanks marked this pull request as ready for review June 8, 2023 14:20
@nibanks nibanks requested a review from a team as a code owner June 8, 2023 14:20
mtfriesen
mtfriesen previously approved these changes Jun 8, 2023
src/manifest/clog.sidecar Outdated Show resolved Hide resolved
@@ -593,7 +607,9 @@ QuicStreamTryCompleteShutdown(
//
// Indicate the stream is completely shut down to the connection.
//
QuicStreamSetReleaseStream(&Stream->Connection->Streams, Stream);
if (!Stream->Flags.DelayFCUpdate || Stream->Flags.HandleClosed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Stream->Flags.HandleClosed

Why is this condition needed?

Copy link
Contributor

@csujedihy csujedihy Jun 9, 2023

Choose a reason for hiding this comment

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

If HandleClose == true, this function, QuicStreamTryCompleteShutdown, is called by QuicStreamClose, right? QuicStreamClose guarantees that QuicStreamSetReleaseStream will be called if DelayFCUpdate is set. Wouldn't we end up calling QuicStreamSetReleaseStream twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

This case is tricky. If an app shuts down (abortively) a stream using the QUIC_STREAM_SHUTDOWN_FLAG_IMMEDIATE flag then we will immediately give them the shutdown complete event and they can close the handle, but MsQuic will continue to track the stream until we've delivered (and received the ACK for) the corresponding stream frames over the wire. In this case, the QuicStreamClose call will run before this code path, but we don't want to release the stream there. Later, when everything is complete, this code path will be run finally, but since the handle has already been closed, we need to release the stream now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

@nibanks nibanks merged commit 4e30ff8 into main Jun 9, 2023
352 of 361 checks passed
@nibanks nibanks deleted the nibanks/delay-stream-fc-to-close branch June 9, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API Area: Core Related to the shared, core protocol logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants