-
Couldn't load subscription status.
- Fork 883
h3: add support for streaming HEADERS with new sequence #2153
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
base: master
Are you sure you want to change the base?
Conversation
Calls to send_request(), send_response(), send_response_with_priority(), and send_additional_headers() can fail with a StreamBlocked error indicating lack of underlying transport capacity. When this occurs, applications are expected to retry the operation when the stream is later reported as writable. However, certain conditions could mean that sufficient capacity might never be made available, effectively permenantly blocking header sends. The root cause of this problem was the choice to enforce that a HEADERS frame is always sent (buffered into a quiche stream) in whole. This change adds new functions related to stream management and header sending to support a streaming send design. These will produce a HEADERS frame that can be sent in whole or in part, depending on available capacity. When a frame is only partly sent, applications are notified and can resume sending using the new continue_partial_headers() method, once the stream is writable. The new functions unify how clients and servers send headers. For a client, the expected sequence is now something like: 0. Decide to initate a request 1. reserve_request_stream() - reserves a stream if limits allow 2. stream_headers() - inititates streaming of a HEADERS frame 3. continue_partial_headers() - if stream_headers() returned Error::PartialHeader For a server, the expected sequence is now something like: 0. Receive a request via the poll() function and decide to respond 1. stream_priority() - set the stream's sending priority per RFC 9218 3. stream_headers() - inititates streaming of a HEADERS frame 4. continue_partial_headers() - if stream_headers return Error::PartialHeader While headers are being streamed, other operations that would cause an HTTP/3 frame to be sent on the stream are prevented. HEADERS frames must be sent completely before other operations are successful. Applications do not need to manage the partial HEADERS buffer, this is dealt with inside quiche. Co-authored-by: Gregor Maier <[email protected]>
e710ee1 to
62513ef
Compare
|
Still needs a bit more testing / hardening but is ready for initial review |
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.
LGTM. Had a couple of smaller questions and comments but I'm really liking this new API.
| } | ||
| } | ||
|
|
||
| #[no_mangle] |
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 you're missing the reserve_request_stream implementation.
| // TODO: retry the request if the error is not fatal | ||
| let stream_id = driver.conn_mut()?.send_request( | ||
| let stream_id = driver.conn_mut()?.reserve_request_stream(qconn)?; | ||
| driver.conn_mut()?.stream_headers( |
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 leave tokio-quiche as-is for now. It can continue to use the old functions until we update all of it to use the new ones.
| Error::MessageError => WireErrorCode::MessageError as u64, | ||
| Error::ConnectError => WireErrorCode::ConnectError as u64, | ||
| Error::VersionFallback => WireErrorCode::VersionFallback as u64, | ||
| Error::PartialHeader => 0x1000, |
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.
nit: do you want to use a larger number here to avoid possible future conflicts?
| // If we received a GOAWAY from the peer, MUST NOT initiate new | ||
| // requests. | ||
| if self.peer_goaway_id.is_some() { | ||
| return Err(Error::StreamCreationError); |
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 we use a different error for this to distinguish it from the self.is_server check about? The "old" send_request used FrameUnexpected
| conn: &mut crate::Connection<F>, stream_id: u64, priority: &Priority, | ||
| ) -> Result<()> { | ||
| // Clamp and shift urgency into quiche-priority space | ||
| let urgency = priority |
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.
nit: should we check if the stream is in self.streams?
| Ok(false) => return Err(Error::StreamBlocked), | ||
| let payload_len = self | ||
| .qpack_encoder | ||
| .encode(headers, &mut frame_payload) |
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 and the above change will make the compiler happy :-)
| .encode(headers, &mut frame_payload) | |
| .encode(headers, frame_payload) |
| /// struct.Connection.html#method.continue_partial_headers | ||
| /// [`FrameUnexpected`]: enum.Error.html#variant.FrameUnexpected | ||
| /// [`StreamBlocked`]: enum.Error.html#variant.StreamBlocked | ||
| /// [`PartialHeader`]: enum.Error.html#variant.PartialHeader |
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.
leave the StreamBlocked and remove the link continue_partial_headers()
| self.send_headers_to_quiche( | ||
| conn, | ||
| stream_id, | ||
| is_trailer_section, |
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 don't think you need to pass is_trailer_section to send_headers_to_quiche(). You can directly call stream.mark_trailers_sent() here (and you don't need trailers_in_flight).
If I remember the H3 spec correctly, after this point the only the valid thing for the user todo on this stream is to call continue_partial_headers() so I don't think we need to distinguish trailers_in_flight from trailers_sent
| /// reported as writable again. | ||
| /// | ||
| /// [`PartialHeader`]: enum.Error.html#variant.PartialHeaders | ||
| /// [`Done`]: enum.Error.html#variant.Done |
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 don't think we ever return Done
| /// [`PartialHeader`]: enum.Error.html#variant.PartialHeader | ||
| pub fn stream_headers<T: NameValue, F: BufFactory>( | ||
| &mut self, conn: &mut super::Connection<F>, stream_id: u64, | ||
| headers: &[T], is_trailer_section: bool, fin: bool, |
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.
is it possible / legal to call this with is_trailer_section == true and fin == false? Should we check for it?
|
|
||
| impl HttpConn for Http3Conn { | ||
| fn retry_partial_headers(&mut self, conn: &mut quiche::Connection) { | ||
| for req in self.reqs.iter_mut().filter(|r| { |
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.
Possible efficiency concern: this filter seems to go through the full list of active streams even if none of them is in the HeadersSendStatus::InProgess state.
Is that a problem?
Related question: is Http3Conn expected to be used in high performance applications?
| ) { | ||
| let mut reqs_done = 0; | ||
| // First retry partial headers | ||
| self.retry_partial_headers(conn); |
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 code proceed to the 2nd loop in cases where continue_partial_headers failed in a retryable way?
I would have expected that at most 1 stream be in the HeadersSendStatus::InProgess state, but it seems like this code allows for multiple streams to move to the HeadersSendStatus::InProgess state
|
|
||
| // Continues sending headers on the given stream. | ||
| int quiche_h3_continue_partial_headers(quiche_h3_conn *conn, | ||
| quiche_conn *quic_conn, uint64_t stream_id); |
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.
weird indentation
|
|
||
| // Initiates streaming of a new HTTP/3 HEADERS frame. | ||
| int quiche_h3_stream_headers(quiche_h3_conn *conn, | ||
| quiche_conn *quic_conn, uint64_t stream_id, |
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.
weird indentation; 1 space missing on this line and next 2.
| /// [`PartialHeader`]: enum.Error.html#variant.PartialHeaders | ||
| /// [`Done`]: enum.Error.html#variant.Done | ||
| pub fn continue_partial_headers( | ||
| &mut self, conn: &mut super::Connection, stream_id: u64, |
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 avoid this stream_id argument? And limit the number of streams that can be in the partial header sent state to 1?
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 we need to allow multiple streams to be in the partial header state. Otherwise stream_headers() would need to return an error if there's already a stream in a partial state and the application would need to deal with retries.
| /// | ||
| /// [`PartialHeader`]: enum.Error.html#variant.PartialHeaders | ||
| /// [`Done`]: enum.Error.html#variant.Done | ||
| pub fn continue_partial_headers( |
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 need a check to make sure that send_body is not called on a stream that is waiting for calls to continue_partial_headers?
Calls to send_request(), send_response(), send_response_with_priority(),
and send_additional_headers() can fail with a StreamBlocked
error indicating lack of underlying transport capacity. When this
occurs, applications are expected to retry the operation when
the stream is later reported as writable.
However, certain conditions could mean that sufficient capacity
might never be made available, effectively permenantly blocking
header sends. The root cause of this problem was the choice to enforce
that a HEADERS frame is always sent (buffered into a quiche stream)
in whole.
This change adds new functions related to stream management and header
sending to support a streaming send design. These will produce a HEADERS
frame that can be sent in whole or in part, depending on available
capacity. When a frame is only partly sent, applications are notified and
can resume sending using the new continue_partial_headers()
method, once the stream is writable.
The new functions unify how clients and servers send headers.
For a client, the expected sequence is now something like:
For a server, the expected sequence is now something like:
While headers are being streamed, other operations that
would cause an HTTP/3 frame to be sent on the stream are
prevented. HEADERS frames must be sent completely before
other operations are successful.
Applications do not need to manage the partial HEADERS
buffer, this is dealt with inside quiche.
Co-authored-by: Gregor Maier [email protected]