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.
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
FIN, RESET_STREAM, RESET_STREAM_AT, STOP_SENDING #589
FIN, RESET_STREAM, RESET_STREAM_AT, STOP_SENDING #589
Changes from 6 commits
c1b89c7
5d6aef0
f21fc8a
dd2cb87
6d76db4
f276b03
39efb04
518e3e4
e1a2b79
5387c55
f29b9df
9ff16c5
fad61e9
899d194
2079552
60dedc3
443ead1
46ae1d6
15d1787
85ee258
3d6d415
abf362c
e5ac755
403bbeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Good question, I don't think so? Fetch doesn't need them clearly.
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 would like to get rid of these, but am happy to do it in a different PR to avoid controversy. If no one wants EndOf Subgroup, we can kill it now.
The edge case I can imagine is if FETCH contents can be SUBSCRIBEd (see #588). There's some ambiguity about FIN if the FETCH ends in the middle of the group. I would be happy to have that uncertainty, personally, but others might disagree.
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'd like to get rid of them too. Setting the object ID is problematic. If we need to, we can retain them for FETCH specifically (though the object ID problem is still there.
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 not sure there's a problem with FETCH? If you ask for the entire group, you get the entire group, if you ask for some Objects in a group, you get what you asked for with no indication as to whether it was the last object.
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.
it's not that there's a "problem" with Fetch; it's that if there are EndOfSubgroup objects, you are potentially sending a LOT of them in response to a FETCH.
Not in this PR, but I think we should get rid of EndOfSubgroup entirely. FETCH response should omit DNE and EndOfGroup status, because they are implied by sequence number gaps and the beginning of the next group, respectively.
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 wrote #596 to remove End Of Subgroup
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.
This sentence is a bit confusing and I'm not sure it's what we want. Why would you send any of these messages and not FIN the stream?
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.
Sometimes the FIN arrives in a separate QUIC packet. This sentence allows you to anticipate the later FIN.
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 still confused why a sender would do this, but I agree it's probably ok to do this.
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.
Can we just remove this, as it is implied, but seems complex.
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 won't die on this hill, but why not explicitly give people permission to do this?
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.
Fair, maybe a reword would make me feel a bit better about it.
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 slightly reworded and moved this paragraph, but I don't know if that solved your problem.
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 did make a suggestion on a way to rewrite the paragraph