-
Notifications
You must be signed in to change notification settings - Fork 205
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
Remove stream reservation #280
Conversation
LGTM. |
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 few small comments
| | ||
| send data/ | ||
| recv data/ | ||
| recv higher 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.
What does "recv higher stream" mean?
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.
See below.
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.
Just to be a little clearer, this is a shorthand for "receive a STREAM frame on a peer-initiated stream with a higher number, given that this is also a peer-initiated stream".
draft-ietf-quic-transport.md
Outdated
the reserved stream transitions to "reserved". | ||
Receiving a STREAM frame on a peer-initiated stream (that is, a packet sent by a | ||
server on an even-numbered stream or a client packet on an odd-numbered stream) | ||
also causes all idle streams with lower numbers to become "open". This could |
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 isn't accurate, since it's only odd/even streams that are marked open. How about '... also causes all lower-numbered idle streams in the same direction to become "open".' ?
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.
Yep, better. This odds and evens thing is a real drag.
draft-ietf-quic-transport.md
Outdated
connection. | ||
A QUIC endpoint cannot reuse a StreamID on a given connection. Streams MUST be | ||
created in sequential order. Open streams can be used in any order. Streams | ||
that are used out of order result in lower-numbered streams being counted as |
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.
"lower-numbered streams in the same direction"
Updated. |
This PR is based on the outcome of the discussion in #174. The observation there is that using stream X implicitly opens stream X-2; since all streams are opened in sequence, it must just be that the packets that mention stream X-2 got delayed or lost somehow.
Once you accept that, it's also reasonable to allow a different order of stream creation (the point at which numbers are allocated) and stream use (when you send packets on a stream). This neatly addresses the issue whereby server push can promise use of streams, but not use them in the order in which they were promised.
There's a catch here, and it's not one that was introduced by this change, if a server promises more streams than the client permits in its concurrent streams limit, the server cannot use streams beyond the limit. I'll open a bug on that.
Closes #182 - that removed only a few of the application actions. This removes all of them.
Closes #174.