-
Notifications
You must be signed in to change notification settings - Fork 204
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
Define terms for application actions #2857
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.
Thanks for doing this Mike, I have quite a few comments.
draft-ietf-quic-http.md
Outdated
responses as a result of having their request terminated abruptly, though | ||
clients can always discard responses at their discretion for other reasons. | ||
not been sent and received. When this is true, a server MAY abort reading the | ||
receiving part of the request stream with error code HTTP_EARLY_RESPONSE, send 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.
Do you think that we can just say "abort reading the request stream" and drop the "receiving part of" piece on the basis that "abort reading" can only apply to the receiving part?
draft-ietf-quic-http.md
Outdated
HTTP_REQUEST_CANCELLED ({{http-error-codes}}). When the client cancels a | ||
response, it indicates that this response is no longer of interest. | ||
Implementations SHOULD cancel requests by aborting both directions of a stream. | ||
Clients can cancel requests by abruptly terminating the sending part of the |
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.
"abruptly terminating" or just "resetting" ?
draft-ietf-quic-http.md
Outdated
The following error codes are defined for use in QUIC RESET_STREAM frames, | ||
STOP_SENDING frames, and CONNECTION_CLOSE frames when using HTTP/3. | ||
The following error codes are defined for use when abruptly terminating the | ||
sending part of streams, aborting reading of the receiving part of streams, or |
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.
As above regarding reference to "parts".
rely on QUIC PADDING frames or employ the reserved frame and stream types | ||
discussed in {{frame-grease}} and {{stream-grease}}. | ||
either rely on transport-layer padding or employ the reserved frame and stream | ||
types discussed in {{frame-grease}} and {{stream-grease}}. |
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.
Aside: we should probably have a conversation about the trade-offs with padding. Transport-layer padding is in some ways superior, like in how it can be granular to a single byte, which reserved streams and frames cannot do. But PADDING frames at the transport aren't retransmitted when lost, which could lead to some interesting attacks if packets can be suppressed by an attacker. If retransmissions aren't padded, boom.
draft-ietf-quic-transport.md
Outdated
|
||
On the sending part of a stream: | ||
|
||
- Attempt to write data, understanding when stream flow control credit |
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.
List construction here implies continuation of a sentence, but the capital letter doesn't fit and there is no joiner.
I think that you want:
On the sending part of a stream, application protocols need to be able to:
- write data, understanding when .... ({{frame-stop-sending}});
- end the stream, ....; and
- reset the stream ... was already ended.
draft-ietf-quic-transport.md
Outdated
any implementation of this version of QUIC MUST expose the ability to perform | ||
the operations described in this section on a QUIC connection. | ||
|
||
When implementing the client role: |
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 list construction comment as above.
draft-ietf-quic-transport.md
Outdated
|
||
When implementing the client role: | ||
|
||
- Open a connection, which begins the exchange described in {{handshake}} |
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.
Add: to enable 0-RTT, and to be informed when 0-RTT has been either accepted or rejected by a server.
draft-ietf-quic-transport.md
Outdated
- If Early Data is supported, embed application-controlled data in the TLS | ||
resumption ticket sent to the client | ||
- If Early Data is supported, retrieve application-controlled data from the | ||
client's resumption ticket and approve/veto accepting Early Data |
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.
and enable rejection of 0-RTT based on that information
I don't like "veto" much here. And we don't need to have an active acceptance involved.
draft-ietf-quic-transport.md
Outdated
In either role: | ||
|
||
- Configure minimum values for the initial number of permitted streams of each | ||
type, as communicated in the transport parameters ({{transport-parameters}}) |
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 there is a more general point, which basically means controlling resource allocation of all types, including streams and flow control.
The point you are looking at here - setting a minimum on the number of streams - is something HTTP needs, so I would add rather than replace.
draft-ietf-quic-transport.md
Outdated
- Keep a connection from silently closing, either by generating PING frames | ||
({{frame-ping}}) or by requesting that the transport send additional frames | ||
before the idle timeout expires ({{idle-timeout}}) | ||
- Cease issuing credit for new streams via MAX_STREAMS frames |
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 the above point about controlling resource allocation.
draft-ietf-quic-transport.md
Outdated
- Keep a connection from silently closing, either by generating PING frames | ||
({{frame-ping}}) or by requesting that the transport send additional frames | ||
before the idle timeout expires ({{idle-timeout}}) | ||
- Cease issuing credit for new streams via MAX_STREAMS frames |
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.
Cease and resume
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 only place where this is required by HTTP is to support GOAWAY; I'd argue that the ability to stop is required, and the ability to resume is something an implementation could choose to expose.
draft-ietf-quic-http.md
Outdated
`initial_max_uni_streams` (three being the minimum value required for the base | ||
HTTP/3 protocol and QPACK), and SHOULD use a value of 1,024 or greater for the | ||
QUIC transport parameter `initial_max_stream_data_uni`. | ||
Endpoints that excessively restrict the number or flow control window of these |
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 reads weird to me, my brain keeps thinking "or" is a typo for "of", which is untrue. I think it could read better to say
"Endpoints that excessively restrict the number of these streams or the size of each stream's flow control window will increase the chance that the remote peer reaches the limit early and becomes blocked."
draft-ietf-quic-http.md
Outdated
unidirectional stream for the HTTP control stream plus the number of | ||
unidirectional streams required by mandatory extensions (three being the minimum | ||
number required for the base HTTP/3 protocol and QPACK), and SHOULD provide at | ||
least 1,024 bytes of flow control credit to each 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.
observation: the change to remove references to Transport Parameters in this paragraph could lead to some interesting unintended issues. Previously it was clear that the problems mentioned were related to small values of initial limits (imposed by TPs). One way to interpet the new text is that it is ok to advertise low values in TPs as long as you send MAX_STREAMS or MAX_STREAM_DATA.
Just wondering if that was the intent of this change, given some of the problems we've seen with these values during interop.
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 point. I personally think they're equivalent, but I believe our consensus was to require it to be in the transport parameters. I added a phrase to clarify this.
draft-ietf-quic-http.md
Outdated
processed. Servers SHOULD NOT increase the QUIC MAX_STREAMS limit after | ||
sending a GOAWAY frame. | ||
connection shutdown. This identifier MAY be zero if no requests were processed. | ||
Servers SHOULD NOT permit additional transport streams after sending a GOAWAY |
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.
"transport stream" is new term not defined elsewhere in the document.
draft-ietf-quic-http.md
Outdated
unnecessarily limit parallelism. | ||
to permit these streams to open, an HTTP/3 server SHOULD configure non-zero | ||
minimum values for the number of permitted streams and the initial flow control | ||
window for each stream. It is RECOMMENDED that at least 100 requests be |
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.
"initial flow control window for each stream" makes it sound as if each stream's flow control window could be configured independently.
draft-ietf-quic-transport.md
Outdated
|
||
Applications also need to be informed of state changes on streams, including | ||
when the peer has initiated, reset, or aborted reading on a stream; when new | ||
data is available; and when data on the stream is blocked due to flow |
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.
when data on the stream is blocked due to flow control
Does this refer to the endpoint's or peer's inability to write?
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.
Thanks for doing this, it looks much better.
draft-ietf-quic-transport.md
Outdated
|
||
Applications also need to be informed of state changes on streams, including | ||
when the peer has initiated, reset, or aborted reading on a stream; when new | ||
data is available; and when data on the stream is blocked due to flow |
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.
Should this be flow or congestion control? And should there be a signal when the stream is unblocked so you can resume sending?
|
||
- open a connection, which begins the exchange described in {{handshake}}; | ||
- enable 0-RTT; and | ||
- be informed when 0-RTT has been accepted or rejected by a server. |
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 certain the application needs to know about 0-RTT accept/reject, but I believe it does need to know when it can start sending 1RTT data?
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 client needs to know because rejection resets the state of all streams; the client has to know whether to start over.
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 absolutely needs to know about this because 0-RTT rejection implies the need to start over in some cases. You can't just treat 0-RTT as "lost".
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 point. I still think the client needs to know when 1-RTT data can be sent, because it may want to send some data in 0-RTT but not other data.
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.
But why does an application need to be able to enable 0-RTT? Isn't 0-RTT optional at transport level, and more so at the app level?
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.
An application needs to be able to control whether 0-RTT is offered; you're correct that this phrasing implies that 0-RTT support is mandatory, which wasn't the intention.
resumption ticket sent to the client; and | ||
- if Early Data is supported, retrieve application-controlled data from the | ||
client's resumption ticket and enable rejecting Early Data based on that | ||
information. |
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.
Servers need to expose whether incoming data is Early Data, correct? ie: for the Early Data header.
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 to stream requirements, but now I'm not so sure that's correct. Early-Data has to be added when the server "forwards a request prior to the completion of the TLS handshake". That would mean the server really only needs a yes/no about the current state of the connection when the request is being forwarded. If the handshake has completed, it doesn't matter that the request was originally early -- it's now retroactively validated as a non-replay by the successful handshake.
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.
Agreed, I think it's only yes/no about the current state of the connection.
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.
An app should also be able to reject incoming connections, for example based on implementation specific criteria such as load or peer IP address.
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.
That seems like a worthwhile feature for a QUIC implementation to expose, but doesn't seem like one we should mandate every QUIC implementation must expose.
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 can agree to that.
draft-ietf-quic-http.md
Outdated
response, it indicates that this response is no longer of interest. | ||
Implementations SHOULD cancel requests by aborting both directions of a stream. | ||
Clients can cancel requests by resetting the request stream with an error code | ||
of HTTP_REQUEST_CANCELLED ({{http-error-codes}}). When the client aborts |
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.
How about "Clients can cancel requests by resetting and aborting the request stream with an error code of HTTP_REQUEST_CANCELLED"? I think that phrasing makes it clearer that you need to reset the write side and abort the read side in order to fully cancel a request?
draft-ietf-quic-http.md
Outdated
Clients can cancel requests by resetting the request stream with an error code | ||
of HTTP_REQUEST_CANCELLED ({{http-error-codes}}). When the client aborts | ||
reading a response, it indicates that this response is no longer of interest. | ||
Implementations SHOULD cancel requests by terminating both directions of 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.
Do we define terminating?
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 terms "clean termination" and "abrupt termination" are given as synonyms for "end the stream" and "reset the stream" in -transport line 361. I should specify the termination type here.
@janaiyengar and @martinthomson, I think this is mergeable from an HTTP standpoint. Since it's technically a requirements change (requiring that functionality be exposed), it probably needs to be marked for a consensus call when you think it's ready. |
|
||
- open a connection, which begins the exchange described in {{handshake}}; | ||
- enable 0-RTT; and | ||
- be informed when 0-RTT has been accepted or rejected by a server. |
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 absolutely needs to know about this because 0-RTT rejection implies the need to start over in some cases. You can't just treat 0-RTT as "lost".
draft-ietf-quic-transport.md
Outdated
stream, it can abort reading the stream and specify an application error code. | ||
|
||
If the stream is in the "Recv or "Size Known" states, the transport SHOULD | ||
signal this by sending a STOP_SENDING frame identifying that stream to prompt |
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 we can lose a few words and make this a little more readable.
signal this by sending a STOP_SENDING frame identifying that stream to prompt | |
signal this by sending a STOP_SENDING frame to prompt |
draft-ietf-quic-transport.md
Outdated
STOP_SENDING frame ({{frame-stop-sending}}) | ||
|
||
Applications also need to be informed of state changes on streams, including | ||
when the peer has initiated, reset, or aborted reading on a stream; when new |
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.
List construction here implies that you can "reset reading on a stream" or that you can observe "initiation of reading on a stream". You probably need to say "when the peer has opened or reset a stream; when a peer aborts reading a stream; when new ..."
type, as communicated in the transport parameters ({{transport-parameters}}); | ||
- control resource allocation of various types, including flow control and the | ||
number of permitted streams of each type; | ||
- identify whether the handshake has completed successfully or is still ongoing |
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 it'd be nice to clarify that a reason for this bullet is to know it can send data in 1-RTT.
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.
Given that hanshake keys must now be held forever, how can it be indicated that a handshake is complete?
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 phrases "handshake completes," "handshake is complete," "handshake has completed," etc. occur throughout the document. If you believe that there can be no such concept, please open a separate issue and explain in detail.
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 objecting to this term being defined, but I do suspect there is an issue if the now infamous PR #3121 gets merged. But, as it hasn't yet, and there is pushback, I'll see where it goes. Defining exactly what is, or might be, wrong requires a deep analysis because one peer can consider a handshake complete without the other peer agreeing, and sometimes that might be fine while in other cases not so much, like path migration.
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 small suggestions, but looks good overall.
a9aaa49
to
5579d57
Compare
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 realize this is merged by now, but I still have a few comments.
- end the stream (clean termination), resulting in a STREAM frame | ||
({{frame-stream}}) with the FIN bit set; and | ||
- reset the stream (abrupt termination), resulting in a RESET_STREAM frame | ||
({{frame-reset-stream}}), even if the stream was already ended. |
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 is unclear - what does it mean that "the stream was already ended"? I would say it is fair for an API to issue a soft error if you try to RESET (abrupt termination) after having issued a FIN request (clean termination), or vice version. Certainly I would not want a QUIC implementation tracking a stream handle indefinitely in case some app decides to reset it eventually.
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.
"End the stream" was the previous bullet, so I think it's reasonably clear what "already ended" should mean in this context. If you look at the sending stream states, this reflects the possibility to transition from "Data Sent" (stream has ended) to "Reset Sent" by sending a RESET_STREAM.
Based on the state diagram, if an implementation was already in the "Data Recvd" state, they wouldn't send a RESET_STREAM, as they're already in a terminal state. Surfacing this through the API would be a reasonable choice. Likewise, there's no path from "Reset Sent" or "Reset Recvd" to "Data Sent"; I think attempting to write to or close a reset stream will always be an error.
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.
So you mean to say that an application should be able to reset a stream at an arbitrary point after the stream has sent a FIN bit? If so, I don't think that is a good idea. In my view that would be an API error of invalid handle or invalid handle state. It's one thing to allow races between RESET and FIN but to require it forever is different.
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.
Rather than having this discussion on a closed PR, let's open a separate issue.
|
||
- open a connection, which begins the exchange described in {{handshake}}; | ||
- enable 0-RTT; and | ||
- be informed when 0-RTT has been accepted or rejected by a server. |
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.
But why does an application need to be able to enable 0-RTT? Isn't 0-RTT optional at transport level, and more so at the app level?
resumption ticket sent to the client; and | ||
- if Early Data is supported, retrieve application-controlled data from the | ||
client's resumption ticket and enable rejecting Early Data based on that | ||
information. |
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.
An app should also be able to reject incoming connections, for example based on implementation specific criteria such as load or peer IP address.
type, as communicated in the transport parameters ({{transport-parameters}}); | ||
- control resource allocation of various types, including flow control and the | ||
number of permitted streams of each type; | ||
- identify whether the handshake has completed successfully or is still ongoing |
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.
Given that hanshake keys must now be held forever, how can it be indicated that a handshake is complete?
This consists of two related commits:
Note that this doesn't remove all references to QUIC frames, but the remaining references are explanatory -- they describe what will happen at the transport layer after taking a particular action, rather than being part of the normative text. (But if you find one of these you think needs to go, please include that in this review.)
This is a design, because exposing this functionality is mandatory. I'm not sure how you would previously have created a usable QUIC implementation without these operations, but you weren't previously required to have them all.
Fixes #2805, closes #2677.