Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/bun-types/globals.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2095,6 +2095,11 @@ interface BunFetchRequestInit extends RequestInit {
* default (h2 is offered iff the flag is on). `"h2"`, `"h3"` and `"h1"`
* are aliases.
*
* Without this option, when the server refuses a request over HTTP/2 with
* `HTTP_1_1_REQUIRED` before it sends a response, Bun sends the request
* again over HTTP/1.1. A request whose body is a stream rejects. With
* `"http2"` the request stays on HTTP/2.
*
* Requires `https`. Not part of the Fetch API specification.
* @experimental
*/
Expand Down
11 changes: 11 additions & 0 deletions src/http/h2_client/ClientSession.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ pub struct ClientSession {
pub(crate) preface_sent: bool,
pub(crate) settings_received: bool,
pub(crate) goaway_received: bool,
/// After GOAWAY(HTTP_1_1_REQUIRED), a request that waits for this connection goes to HTTP/1.1.
pub(crate) http1_required: bool,
/// Set when the HPACK encoder's dynamic table has diverged from the
/// server's view (writeRequest failed mid-encode). Existing siblings whose
/// HEADERS already went out are unaffected, but no new stream may be
Expand Down Expand Up @@ -395,6 +397,7 @@ impl ClientSession {
preface_sent: false,
settings_received: false,
goaway_received: false,
http1_required: false,
encoder_poisoned: false,
delivering: false,
stream_progressed: false,
Expand Down Expand Up @@ -507,6 +510,10 @@ impl ClientSession {
} else if self.has_headroom() {
self.attach(client);
} else {
if self.http1_required {
// A request that names h2 keeps its protocol.
client.pin_to_http1();
}
client.h2_retry_after_coalesce();
}
}
Expand Down Expand Up @@ -1207,6 +1214,10 @@ impl ClientSession {
if let Some(err) = stream.fatal_error {
stream.client = None;
client.h2 = None;
if stream.http1_required && stream.status_code == 0 && client.can_retry_over_http1() {
client.retry_over_http1();
return true;
}
// Only transparently retry when the server refused the stream
// before producing any of it (REFUSED_STREAM after HEADERS would
// be a server bug, but retrying then re-streams a body prefix
Expand Down
3 changes: 3 additions & 0 deletions src/http/h2_client/Stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ pub struct Stream {
/// or final status arrives.
pub(crate) awaiting_continue: bool,
pub(crate) fatal_error: Option<Error>,
/// Refused with HTTP_1_1_REQUIRED. A GOAWAY in the same read overwrites only `fatal_error`.
pub(crate) http1_required: bool,
/// DATA bytes consumed since the last WINDOW_UPDATE for this stream.
pub(crate) unacked_bytes: u32,
/// Σ DATA payload bytes (post-padding) for §8.1.1 Content-Length check —
Expand Down Expand Up @@ -144,6 +146,7 @@ impl Stream {
headers_end_stream: false,
awaiting_continue: false,
fatal_error: None,
http1_required: false,
unacked_bytes: 0,
data_bytes_received: 0,
send_window,
Expand Down
8 changes: 8 additions & 0 deletions src/http/h2_client/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,7 @@ fn dispatch_frame(
}
_ => Some(crate::Error::HTTP2StreamReset),
};
stream.http1_required |= code == wire::ErrorCode::HTTP_1_1_REQUIRED.0;
}
FT_GOAWAY => {
if stream_id != 0 {
Expand All @@ -532,11 +533,14 @@ fn dispatch_frame(
wire::UInt31WithReserved::from_bytes(&payload[0..4]).uint31();
let code: u32 = wire::u32_from_bytes(&payload[4..8]);
let graceful = code == wire::ErrorCode::NO_ERROR.0;
let http1_required = code == wire::ErrorCode::HTTP_1_1_REQUIRED.0;
session.http1_required |= http1_required;
let last_id = session.goaway_last_stream_id;
for &s_ptr in session.streams.values() {
// SAFETY: stream pointer valid for session lifetime.
let s = stream_mut(s_ptr);
if s.id > last_id {
s.http1_required |= http1_required;
s.fatal_error = Some(if graceful {
crate::Error::HTTP2RefusedStream
} else {
Expand All @@ -547,6 +551,10 @@ fn dispatch_frame(
// still complete successfully" — don't discard a
// response that already finished in this same read.
s.fatal_error = Some(crate::Error::HTTP2GoAway);
// The peer may have processed the request: only an idempotent one goes again.
if http1_required && s.client_ref().is_some_and(|c| c.method.is_idempotent()) {
s.http1_required = true;
}
}
}
}
Expand Down
30 changes: 28 additions & 2 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2187,11 +2187,37 @@ impl<'a> HTTPClient<'a> {
self.start_::<true>();
}

/// A request that names its protocol keeps it, and only a `Bytes` body can be sent again.
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 refused the request with HTTP_1_1_REQUIRED before it sent a response.
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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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. find_in (HTTPContext.rs:869-875) then hands that socket to any later request that offers H1OrH2, and each use re-parks it. Fix: keep a socket opened by retry_over_http1 out of the pool, or make the pool prefer an idle h2 session over an h1 socket for H1OrH2 requests, so only requests pinned to h1 take it. The PR asks whether this is acceptable; with the 5-minute idle default (HTTPContext.rs:781) and steady traffic the socket never expires.

Why this was flagged

A server answers one request with RST_STREAM(HTTP_1_1_REQUIRED) while the h2 flag is on. ClientSession.rs:1218-1219 calls retry_over_http1, lib.rs:2199-2204, which pins the request and restarts it over a new HTTP/1.1 TLS connection; on completion that socket is released to the keep-alive pool like any h1 socket. A later request without protocol calls alpn_offer() -> H1OrH2; connect (HTTPContext.rs:1124) asks existing_socket, and find_in returns the first slot that passes HTTPContext.rs:869-875, where an h1 socket is accepted for H1OrH2. Each such use re-parks the socket and re-arms the 5-minute timer (HTTPContext.rs:779-783), so under continuous traffic the origin stays on HTTP/1.1 for one request at a time, with no multiplexing, until the socket is idle for 5 minutes. On the base branch only an explicit protocol: "http1.1" could put an h1 socket in the pool while the flag is on, so this did not occur for ordinary requests. The PR description flags this and asks if it is acceptable; no code bounds it.

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: /next is answered "200 over h1" although an idle h2 session is in the pool. find_in (HTTPContext.rs:867-875) accepts the h1 socket for any H1OrH2 caller.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

  • A request takes the idle HTTP/1.1 connection only when no h2 connection is in use. connect in HTTPContext.rs checks active_h2_sessions before the pool, so concurrent requests still join an active h2 connection.
  • The two fixes have opposite costs. If the pool prefers h2, an origin that refuses every request with HTTP_1_1_REQUIRED pays one refused h2 attempt for each request. If the connection of the resend stays out of the pool, each refused request pays a new TLS handshake.
  • Chromium goes the other way. After one HTTP_1_1_REQUIRED it uses HTTP/1.1 for the origin (SetHTTP11Required). That needs a table per origin, which is a separate change.

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 find_in. I can add it here.

}

/// `can_offer_h2` and `can_try_h3_alt_svc` refuse the request from here on, redirects included.
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""),
Expand Down Expand Up @@ -4120,7 +4146,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
Expand Down
1 change: 1 addition & 0 deletions src/http_types/h2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ impl ErrorCode {
pub const CANCEL: Self = Self(0x8);
pub const COMPRESSION_ERROR: Self = Self(0x9);
pub const ENHANCE_YOUR_CALM: Self = Self(0xb);
pub const HTTP_1_1_REQUIRED: Self = Self(0xd);
}

#[repr(transparent)]
Expand Down
Loading
Loading