Repository navigation
http(h2): send the request again over HTTP/1.1 after HTTP_1_1_REQUIRED #46259
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: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2187,11 +2187,41 @@ impl<'a> HTTPClient<'a> { | |
| self.start_::<true>(); | ||
| } | ||
|
|
||
| /// Whether `retry_over_http1` applies. A request that names its protocol | ||
| /// has no HTTP/1.1 to go to, and only a `Bytes` body can be sent again. | ||
|
robobun marked this conversation as resolved.
Resolved
|
||
| pub(crate) fn can_retry_over_http1(&self) -> bool { | ||
| self.flags.forced_protocol.is_none() | ||
| && matches!(self.state.original_request_body, HTTPRequestBody::Bytes(_)) | ||
| } | ||
|
|
||
| /// The peer ended the stream of the request with HTTP_1_1_REQUIRED before | ||
| /// it sent a response. | ||
|
robobun marked this conversation as resolved.
Resolved
|
||
| pub(crate) fn retry_over_http1(&mut self) { | ||
| debug_assert!(self.h2.is_none()); | ||
| debug_assert!(self.can_retry_over_http1()); | ||
| self.pin_to_http1(); | ||
| self.restart_from_top(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Users running with the h2 flag can lose HTTP/2 for an origin after a single HTTP_1_1_REQUIRED fallback, for as long as traffic keeps the fallback socket alive. The resent request parks its HTTP/1.1 socket in the keep-alive pool on completion. Why this was flaggedA server answers one request with RST_STREAM(HTTP_1_1_REQUIRED) while the h2 flag is on. ClientSession.rs:1218-1219 calls Verification: The PR description's "Downsides" names it and the new test "an HTTP/1.1 connection that became idle before the h2 connection carries the next request" (test/js/web/fetch/fetch-http2-client.test.ts:2980-3009) pins it:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left this as it is. The question is in the Downsides of the description, for a maintainer to decide.
If a maintainer wants the pool to keep a connection that never offered h2 away from a request that offers it, that is one flag on the pooled socket and one check in |
||
| } | ||
|
|
||
| /// Keep the rest of this request on HTTP/1.1: `can_offer_h2` and | ||
| /// `can_try_h3_alt_svc` refuse it from here on, redirect hops included. | ||
| /// A request that names its protocol stays as it is. | ||
|
robobun marked this conversation as resolved.
Resolved
|
||
| pub(crate) fn pin_to_http1(&mut self) { | ||
| if self.flags.forced_protocol.is_none() { | ||
| self.flags.forced_protocol = Some(Protocol::Http1_1); | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn retry_from_h2(&mut self) { | ||
| debug_assert!(self.h2.is_none()); | ||
| self.h2_retries += 1; | ||
| self.restart_from_top(); | ||
| } | ||
|
|
||
| /// Start the request again through `start()`. The body can be sent again from its first byte. | ||
| fn restart_from_top(&mut self) { | ||
| self.unregister_abort_tracker(); | ||
| self.flags.protocol = Protocol::Http1_1; | ||
| self.h2_retries += 1; | ||
| let body = core::mem::replace( | ||
| &mut self.state.original_request_body, | ||
| HTTPRequestBody::Bytes(b""), | ||
|
|
@@ -4120,7 +4150,7 @@ impl<'a> HTTPClient<'a> { | |
| // PendingConnect that the rest of this loop would re-coalesce | ||
| // onto (which would serialise N cold fetches into N | ||
| // sequential handshakes). The origin already chose h1 once. | ||
| waiter.flags.forced_protocol = Some(Protocol::Http1_1); | ||
| waiter.pin_to_http1(); | ||
| waiter.start_::<true>(); | ||
| } | ||
| // The first waiter becomes the new leader; the rest re-coalesce | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.