-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conversation
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.
LG overall, some small comments.
draft-ietf-moq-transport.md
Outdated
objects when it , it MUST use a RESET_STREAM or RESET_STREAM_AT | ||
{{!I-D.draft-ietf-quic-reliable-stream-reset}} frame. This includes early | ||
termination of subscription due to an UNSUBSCRIBE message, a sender's | ||
decision to abandon the subscription before the Group is complete, or a |
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.
What if you give up on delivering a Subgroup due to Delivery Timeout?
For abandoning a subscription early, that corresponds to sending a SUBSCRIBE_DONE, correct?
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 made Delivery Timeout as a reason explicit, instead of implicit.
As another issue makes clear, I don't know what SUBSCRIBE_DONE means. Is it a "Reset" or a "Fin" of a subscription? Or both?
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 typically a 'Fin' of a subscription. Maybe one sends a SUBSCRIBE_DONE when all subgroups have been Fin'd?
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.
Well if it's a "subscribe fin", then it ends on a group boundary and all the streams will naturally close with their own FIN, with no special mechanism required.
draft-ietf-moq-transport.md
Outdated
An Original Publisher MAY create EndOfSubgroup objects to provide further | ||
indication that streams are OK to close. | ||
|
||
[TODO: Do we still need EndOfSubgroup objects?] |
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.
[TODO: Do we still need EndOfSubgroup objects?] | |
[TODO: Do we still need EndOfSubgroup objects?] |
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
draft-ietf-moq-transport.md
Outdated
as to reliably deliver the objects it has while signaling that other objects | ||
exist. | ||
|
||
A relay MAY also use EndOfGroup messages in a Subgroup stream to close streams |
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
Co-authored-by: ianswett <[email protected]>
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.
Individual Review
draft-ietf-moq-transport.md
Outdated
promptly frees system resources and often unlocks flow control credit to open | ||
more streams. | ||
|
||
If a sender has delivered all objects in a Subgroup, except any objects that |
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.
a sender has delivered all objects in a Subgroup
Seems like this only applies to the original publisher, right? For a relay, you need to explain how they know they have delivered all the objects in a subgroup.
Also, what does "delivered" mean? I think you mean they've been committed to the QUIC stream, rather than transmitted or acknowledged.
Should we say "requested" objects to be inclusive of subscribe filters?
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 PR is mostly about how the relay knows it has received everything in the Subgroup!
I'll clarify the meaning of "delivered".
Subscribe filters can only omit the beginning of a subgroup, not the end, hence the "before the beginning of a subscription" excecption.
draft-ietf-moq-transport.md
Outdated
fall before the beginning of a subscription, it MUST close the stream | ||
with a FIN. | ||
|
||
If a sender closes the stream before verifying it has delivered all such |
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.
Same comment here regarding "verifying it has delivered".
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.
OK, I changed this to enumerate three cases:
sent everything and wait for reliable delivery: FIN
didn't send everything: RESET_STREAM
sent everything, don't wait for delivery: FIN followed by RESET_STREAM.
draft-ietf-moq-transport.md
Outdated
An Original Publisher MAY create EndOfSubgroup objects to provide further | ||
indication that streams are OK to close. | ||
|
||
[TODO: Do we still need EndOfSubgroup objects?] |
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.
Some editorial comments, but this still LG from a functional perspective.
draft-ietf-moq-transport.md
Outdated
An Original Publisher MAY create EndOfSubgroup objects to provide further | ||
indication that streams are OK to close. | ||
|
||
[TODO: Do we still need EndOfSubgroup objects?] |
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.
draft-ietf-moq-transport.md
Outdated
can forward stream FINs to its own subscribers once those objects have been | ||
sent. A relay MAY use EndOfGroup, EndOfSubgroup, GroupDoesNotExist, and | ||
EndOfTrack objects to close streams even if the FIN has not arrived, as further | ||
objects on the stream would be a protocol violation. |
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.
draft-ietf-moq-transport.md
Outdated
as to reliably deliver the objects it has while signaling that other objects | ||
exist. | ||
|
||
A relay MAY also use EndOfGroup messages in a Subgroup stream to close streams |
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.
Co-authored-by: ianswett <[email protected]>
Co-authored-by: ianswett <[email protected]>
Co-authored-by: ianswett <[email protected]>
Co-authored-by: ianswett <[email protected]>
This is a dependency of Martin's Stream FIN/RESET PR, #589
Co-authored-by: ianswett <[email protected]>
Co-authored-by: afrind <[email protected]>
Co-authored-by: afrind <[email protected]>
This is a dependency of Martin's Stream FIN/RESET PR, #589
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've tried to make any editorial changes so this is ready for merging I believe.
LGTM |
We don't discuss how to use it anyway. This was suggested in #589
This is explicitly not dealing with FETCH at all, but it won't be hard for me to write something similar for the FETCH stream once the rubble settles on that proposal.