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
Possible SETTINGS_ACK proposal #84
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Let's say that the opening of a stream is delayed (loss looks like a delay for this stuff). Stream N gets through, but stream N-2 takes another RTT or so to open. What is the receiver of the SETTINGS frame expected to do in this case? Is it expected to play out the SETTINGS_ACK when the stream finally makes it through?
The problem here is that stream N-2 might have been skipped by the other side. How does a receiver know what to do with that uncertainty? It can't hold onto the SETTINGS_ACK indefinitely.
I think that we need to mandate no stream skipping in this case.
The next problem is in correlating the SETTINGS_ACK with the SETTINGS. I think that your scheme almost works, but we probably want to include some text on how this correlation is expected to work:
Where this doesn't work is when the receiver of a SETTINGS frame opens a stream. Say a server sends SETTINGS at and then a client opens stream 7. The server cannot know whether stream 7 was open at the time that the client received the SETTINGS frame. Even if the opening frame on stream 7 is in the same packet as the client's SETTINGS_ACK, there is no good way to know what order the client actually performed these two actions. Thus, if stream 7 is still open when a second SETTINGS is sent, the SETTINGS_ACK that the server receives on stream 7 could correspond to either settings.
The only fix I'm aware of is to set start AND end markers on the control stream SETTINGS_ACK.
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.
The first case is the corner-case from line 572 -- because QUIC mandates that streams be opened "sequentially," stream-skipping is already prohibited. (Should that be made clearer?) The fact that I've seen stream N opened means N-2 is in-flight, even if I haven't seen it yet. That means, having received a SETTINGS frame, I can go ahead and send a SETTINGS_ACK on N-2 without having yet received a frame because it's not out-of-order at the remote side.
Your suggested correlation text looks a lot like what I have at line 576 about what's needed to consider the ACK "finished." I might need to add a note that all previous SETTINGS frames have to be "finished" before a subsequent one can be -- ACKs on-stream are fully ordered.
I don't see the issue when the receiver of SETTINGS opens the stream. There are two possible orderings:
- SETTINGS processed before stream created. Then the SETTINGS_ACK on stream 3 will not cover the new stream, and the stream abides by the new settings from the start.
What am I missing?
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 think that you have it. Thanks for walking through it with me. The sequential thing definitely needs work. "Sequential" just means in order, which doesn't correspond to a mandate to use every number.
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.
Added some text to the Stream Usage section explicitly requiring no stream-skipping, just to be thorough.