-
-
Notifications
You must be signed in to change notification settings - Fork 321
Implement HTTP/2 informational responses (1xx) support #865
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
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 run the tests and it seems to work, and the diff seems to make some sense to me, although I would like someone more familiar with Hyper to have a look at it.
However, I already see this could benefit from at least running rustfmt and fixing one doctest (details below). Also the commit messages should be changed to conform to the commit guidelines.
| /// # Examples | ||
| /// ```rust | ||
| /// use h2::server; | ||
| /// use http::{Response, StatusCode}; | ||
| /// | ||
| /// // Send 100 Continue before processing request body | ||
| /// let continue_response = Response::builder() | ||
| /// .status(StatusCode::CONTINUE) | ||
| /// .header("x-custom", "value") | ||
| /// .body(()) | ||
| /// .unwrap(); | ||
| /// send_response.send_informational(continue_response)?; | ||
| /// | ||
| /// // Later send the final response | ||
| /// let final_response = Response::builder() | ||
| /// .status(StatusCode::OK) | ||
| /// .body(()) | ||
| /// .unwrap(); | ||
| /// let stream = send_response.send_response(final_response, false)?; | ||
| /// ``` |
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 doctest is failing. You should create a send_response instance and hide the line from the docs if you wish by starting the line with # as described in Rust doctest documentation.
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, @vikanezrimaya. I have fixed the doctest, and run with cargo test --doc explicitly.
b7ec555 to
835bb8f
Compare
pablocm
left a comment
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.
My overall feedback:
- About naming/terminology: The RFC9110 I think we're conflating two related concepts: The word "Informational" defines the range of 1XX status codes, and "interim" is used to describe non-final responses. I think it would be conceptually clearer to use the latter to describe some of the logic, e.g.
send_interim_headersinstead ofsend_informational_headers_direct. Thoughts? - Can you provide integration tests in
tests/h2-tests/showing example happy paths and also corner cases?
Full disclosure -- the author and I are work colleagues. I didn't participate in the coding but do have vested interested in seeing this feature merged.
src/server.rs
Outdated
| /// Informational responses are used to provide early feedback to the client | ||
| /// before the final response is ready. Common examples include: | ||
| /// - 100 Continue: Indicates the client should continue with the request | ||
| /// - 102 Processing: Indicates the server is processing the request | ||
| /// - 103 Early Hints: Provides early hints about resources to preload |
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.
102 Processing is non-standard and deprecated, I think it should be removed from the examples.
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.
Addressed. There is not no instance of 102 in any example. Since it is deprecated, it did not work when I tried implementing it. I have updated the doc.
| /// let continue_response = Response::builder() | ||
| /// .status(StatusCode::CONTINUE) | ||
| /// .header("x-custom", "value") | ||
| /// .body(()) | ||
| /// .unwrap(); | ||
| /// send_response.send_informational(continue_response)?; |
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.
Since interim responses are not allowed to have a body, using .body(()) is mandatory. If a caller attempts to add a body, should h2 treat as an error/fail state?
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.
send_informational takes a Response<()> so you can't use anything but (), did you mean about another place in the code?
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.
You're correct, it's enforced by the type system. Disregard my comment!
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.
- About naming/terminology: The RFC9110 I think we're conflating two related concepts: The word "Informational" defines the range of 1XX status codes, and "interim" is used to describe non-final responses. I think it would be conceptually clearer to use the latter to describe some of the logic, e.g. send_interim_headers instead of send_informational_headers_direct. Thoughts?
You're absolutely correct. RFC 9110 Section 15.2 uses "informational" to describe 1xx status codes as a category, while "interim response" describes the semantic meaning of non-final responses. I will keep the public API as send_informational() which aligns with HTTP semantics (1xx = informational status codes), but change the internal functions name send_informational_headers_direct() to send_interim_informational_headers() for better semantic accuracy.
- Can you provide integration tests in tests/h2-tests/ showing example happy paths and also corner cases?
Addressed! It covers 6 scenarios. Let me know if you think of any other scenarios.
Happy Path Tests:
send_100_continue- Basic 100 Continue functionalitysend_103_early_hints- 103 Early Hints with Link headerssend_multiple_informational_responses- Multiple 1xx responses in sequenceclient_poll_informational_responses- Client-side polling APIinformational_responses_with_body_streaming- Streaming scenarios
Corner Case Tests:
ignore_invalid_informational_status- Invalid status code handling (sends 200 as informational, should be ignored)
More example based and end testing are added in hyper pr, which will be released after merging this one.
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.
11.
ignore_invalid_informational_status- Invalid status code handling (sends 200 as informational, should be ignored)
I don't think I agree with that, there is a concept of user error in the crate and this should probably be one.
|
FWIW, there is also a new 104 status code for resumable uploads being standardized, so you can consider that another use case: https://www.ietf.org/archive/id/draft-ietf-httpbis-resumable-upload-10.html#section-5 |
Add support for HTTP/2 informational responses (1xx status codes) including 103 Early Hints. This enables servers to send preliminary headers before the final response, improving client performance through early resource discovery and connection establishment. Changes include: - extend client and server APIs to handle interim informational responses - update stream state management for 1xx responses - add test for interim informational response scenarios This implementation follows RFC 7540 and RFC 8297 specifications for HTTP/2 informational responses handling.
@LPardue for sure! The PRs are the backbone for the informational codes support for hyper. I will add 104 specific examples once 103 related code is merged. |
Add support for HTTP/2 informational responses, enabling servers to send multiple 1xx status code responses before the final response and clients to receive them separately.
Key Features Added
Client-side Support
poll_informational()method toResponseFuturefor polling 1xx responsesServer-side Support
send_informational()method toSendResponsewith documentationProtocol Implementation
InformationalHeadersevent type to distinguish from regular headerssend_informational_headers_direct()for sending without state changesChanged Files
* send_100_continue: Basic 100 Continue functionality
* send_103_early_hints: 103 Early Hints with Link headers
* send_multiple_informational_responses: Multiple 1xx responses in sequence
* ignore_invalid_informational_status: Invalid status code handling
* client_poll_informational_responses: Client-side polling API
* informational_responses_with_body_streaming: Streaming scenarios
Use Cases Supported
Technical Notes
Resolves support for HTTP 103 Early Hints and 100 Continue HTTP/2 informational responses.