From a325da0310558e24a6317a8b319d9f3da3c955f2 Mon Sep 17 00:00:00 2001 From: Elrendio Date: Thu, 8 Oct 2026 00:54:44 +0200 Subject: [PATCH 1/2] fix: replenish empty DATA frame limit with payload Problem Since #945, empty non-final DATA frames are counted over the lifetime of the connection, and the 101st one closes it with ENHANCE_YOUR_CALM however much data arrived in between. A padding-only frame counts as empty, and some senders emit them routinely: Google's front end (`server: ESF`, *.googleapis.com) sends PADDED DATA frames that carry only padding right after full 16 KiB DATA frames. A pooled client connection to such a server crosses the limit after enough responses and closes itself with GOAWAY(ENHANCE_YOUR_CALM), failing every in-flight request on it. Users of h2 cannot change that sender. Refs #964. Solution Charge each empty non-final DATA frame DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD against a budget worth MAX_RECV_EMPTY_DATA_FRAMES such frames, and let non-final DATA frames larger than the threshold earn back their payload beyond it. This is the cost 0.4.16 to 0.4.18 charged empty frames through the DATA frame budget. The budget stays separate from the DATA frame budget, as #945 intended, and does not follow the automatic budget. - A burst of empty frames is still rejected on the 101st frame. - Frames of up to 256 bytes and final frames earn nothing back. - Each empty frame beyond the burst costs the peer at least 256 bytes of flow-controlled payload. This reverses the #945 unit test `large_data_frames_do_not_replenish_empty_data_frame_limit`, renamed to `large_data_frames_replenish_empty_data_frame_budget`. Validation - New integration test `padded_empty_data_frames_between_large_data_frames_do_not_exhaust_budget`: 150 responses on one connection, each a 1024-byte DATA frame, a padding-only DATA frame and an empty END_STREAM frame. It fails on master with GoAway(too_many_data_frames) on stream 201 (the 101st response) and passes with this change. - New unit tests cover the refill being capped at the burst, frames of up to 256 bytes earning nothing, and the target window leaving the empty frame budget unchanged. - `too_many_empty_data_frames_sends_goaway`, `too_many_padded_empty_data_frames_sends_goaway` and `empty_data_frames_do_not_consume_data_frame_budget` pass unchanged. - `cargo fmt --all -- --check`, `cargo clippy --all-features --lib -- -D warnings`, `cargo test --all-features` and `cargo test -p h2-tests` pass, except `clear_recv_buffer_caps_capacity_before_overflow`, which also fails on master. - A reqwest 0.13 / hyper 1.11 client fetching https://sheets.googleapis.com/$discovery/rest?version=v4 600 times on one connection fails at request 35 with h2 0.4.19, and completes 600/600 with this change after receiving 1,721 padding-only frames. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/client.rs | 5 ++- src/proto/mod.rs | 3 ++ src/proto/streams/counts.rs | 56 ++++++++++++++++------- src/server.rs | 5 ++- tests/h2-tests/tests/stream_states.rs | 65 +++++++++++++++++++++++++++ 5 files changed, 114 insertions(+), 20 deletions(-) diff --git a/src/client.rs b/src/client.rs index 9d0ff31d9..dce27aec0 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1161,8 +1161,9 @@ impl Builder { /// /// Small DATA frames consume this budget. The budget is restored when /// buffered frames are consumed by the application, while sufficiently - /// large frames may also restore budget. Empty DATA frames are limited - /// separately and do not consume this budget. + /// large frames may also restore budget. Empty DATA frames do not consume + /// this budget: they have a separate, fixed allowance that sufficiently + /// large frames also restore. /// /// When this budget is exhausted, the connection is closed with /// `ENHANCE_YOUR_CALM`. diff --git a/src/proto/mod.rs b/src/proto/mod.rs index 5e3b4f40a..de566b481 100644 --- a/src/proto/mod.rs +++ b/src/proto/mod.rs @@ -37,6 +37,9 @@ pub const DEFAULT_LOCAL_RESET_COUNT_MAX: usize = 1024; // smaller than this consume more internal bookkeeping than useful data. pub const DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD: usize = 256; pub const DEFAULT_DATA_FRAME_BUDGET: usize = DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD * 100; +// Number of empty, non-final DATA frames accepted in a burst. Each non-final +// DATA frame earns back one more for every DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD +// bytes of payload beyond that threshold. pub const MAX_RECV_EMPTY_DATA_FRAMES: usize = 100; // RFC 9113 suggests allowing at minimum 100 streams, it seems reasonable to // by default allow a portion of that to be remembered as reset for some time. diff --git a/src/proto/streams/counts.rs b/src/proto/streams/counts.rs index f74aa14a7..ab23d3ecb 100644 --- a/src/proto/streams/counts.rs +++ b/src/proto/streams/counts.rs @@ -83,9 +83,12 @@ pub(super) struct Counts { /// Whether the DATA frame budget follows the target connection window. data_frame_budget_is_auto: bool, - /// Number of empty, non-final DATA frames received over the lifetime of - /// the connection. - num_recv_empty_data_frames: usize, + /// connection-level budget for empty, non-final DATA frames. + /// + /// Each empty frame is charged the full overhead threshold, while larger + /// non-final frames earn back their payload beyond the threshold, so a + /// peer can only sustain empty frames by also sending payload. + empty_data_frame_budget: Budget, } impl Counts { @@ -105,7 +108,9 @@ impl Counts { num_local_error_reset_streams: 0, data_frame_budget: Budget::new(config.data_frame_budget.resolve(None)), data_frame_budget_is_auto: config.data_frame_budget.is_auto(), - num_recv_empty_data_frames: 0, + empty_data_frame_budget: Budget::new( + MAX_RECV_EMPTY_DATA_FRAMES * DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD, + ), } } @@ -119,20 +124,15 @@ impl Counts { /// Records the framing overhead of a DATA frame. pub fn record_data_frame(&mut self, payload_len: usize) -> Result<(), BudgetExhausted> { if payload_len == 0 { - self.num_recv_empty_data_frames = self - .num_recv_empty_data_frames - .checked_add(1) - .ok_or(BudgetExhausted)?; - if self.num_recv_empty_data_frames > MAX_RECV_EMPTY_DATA_FRAMES { - return Err(BudgetExhausted); - } - Ok(()) + self.empty_data_frame_budget + .consume(DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD) } else if payload_len < DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD { self.data_frame_budget .consume(DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD - payload_len) } else { - self.data_frame_budget - .replenish(payload_len - DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD); + let surplus = payload_len - DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD; + self.data_frame_budget.replenish(surplus); + self.empty_data_frame_budget.replenish(surplus); Ok(()) } } @@ -442,6 +442,10 @@ mod tests { ); auto.set_target_connection_window_size(1024 * 1024); assert_eq!(auto.data_frame_budget.max, 512 * 1024); + assert_eq!( + auto.empty_data_frame_budget.max, + MAX_RECV_EMPTY_DATA_FRAMES * DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD + ); for _ in 0..1_000 { auto.record_data_frame(171).unwrap(); } @@ -494,13 +498,33 @@ mod tests { } #[test] - fn large_data_frames_do_not_replenish_empty_data_frame_limit() { + fn large_data_frames_replenish_empty_data_frame_budget() { + let mut counts = counts(); + + // Each frame earns back twice what one empty frame costs, so an + // uncapped refill would build up extra allowance. + for _ in 0..MAX_RECV_EMPTY_DATA_FRAMES * 10 { + counts.record_data_frame(0).unwrap(); + counts + .record_data_frame(DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD * 3) + .unwrap(); + } + + // Replenishment does not accumulate beyond the initial allowance. + for _ in 0..MAX_RECV_EMPTY_DATA_FRAMES { + counts.record_data_frame(0).unwrap(); + } + assert!(counts.record_data_frame(0).is_err()); + } + + #[test] + fn small_data_frames_do_not_replenish_empty_data_frame_budget() { let mut counts = counts(); for _ in 0..MAX_RECV_EMPTY_DATA_FRAMES { counts.record_data_frame(0).unwrap(); counts - .record_data_frame(DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD * 2) + .record_data_frame(DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD - 1) .unwrap(); } assert!(counts.record_data_frame(0).is_err()); diff --git a/src/server.rs b/src/server.rs index 931661a19..91d6bd411 100644 --- a/src/server.rs +++ b/src/server.rs @@ -1055,8 +1055,9 @@ impl Builder { /// /// Small DATA frames consume this budget. The budget is restored when /// buffered frames are consumed by the application, while sufficiently - /// large frames may also restore budget. Empty DATA frames are limited - /// separately and do not consume this budget. + /// large frames may also restore budget. Empty DATA frames do not consume + /// this budget: they have a separate, fixed allowance that sufficiently + /// large frames also restore. /// /// When this budget is exhausted, the connection is closed with /// `ENHANCE_YOUR_CALM`. diff --git a/tests/h2-tests/tests/stream_states.rs b/tests/h2-tests/tests/stream_states.rs index e302f2328..1ec039744 100644 --- a/tests/h2-tests/tests/stream_states.rs +++ b/tests/h2-tests/tests/stream_states.rs @@ -232,6 +232,71 @@ async fn too_many_padded_empty_data_frames_sends_goaway() { join(mock, h2).await; } +#[tokio::test] +async fn padded_empty_data_frames_between_large_data_frames_do_not_exhaust_budget() { + h2_support::trace_init!(); + + // More streams than the burst of empty DATA frames allowed, with one empty + // frame per stream. + const NUM_STREAMS: u32 = 150; + const BODY_LEN: usize = 1024; + // Large enough that the capacity released automatically never reaches the + // WINDOW_UPDATE threshold, so the client sends no further WINDOW_UPDATE + // frames. + const CONNECTION_WINDOW: u32 = 1 << 20; + + let (io, mut srv) = mock::new(); + + let mock = async move { + let settings = srv.assert_client_handshake().await; + assert_default_settings!(settings); + srv.recv_frame(frames::window_update(0, CONNECTION_WINDOW - 65_535)) + .await; + + for i in 0..NUM_STREAMS { + let stream_id = 1 + i * 2; + srv.recv_frame( + frames::headers(stream_id) + .request("GET", "https://http2.akamai.com/") + .eos(), + ) + .await; + srv.send_frame(frames::headers(stream_id).response(200)) + .await; + srv.send_frame(frames::data(stream_id, vec![0; BODY_LEN])) + .await; + // A zero padding length byte with no payload, as some servers + // emit between the DATA frames of a body. + srv.send_frame(frames::data(stream_id, [0]).padded()).await; + srv.send_frame(frames::data(stream_id, "").eos()).await; + } + }; + + let h2 = async move { + let (mut client, mut h2) = client::Builder::new() + .initial_connection_window_size(CONNECTION_WINDOW) + .handshake::<_, Bytes>(io) + .await + .unwrap(); + + for _ in 0..NUM_STREAMS { + let request = Request::builder() + .uri("https://http2.akamai.com/") + .body(()) + .unwrap(); + let response = client.send_request(request, true).unwrap().0; + let response = h2.drive(response).await.unwrap(); + let body = h2.drive(util::concat(response.into_body())).await; + assert_eq!(body.unwrap().len(), BODY_LEN); + } + + drop(client); + h2.await.unwrap(); + }; + + join(mock, h2).await; +} + #[tokio::test] async fn too_many_small_data_frames_sends_goaway() { h2_support::trace_init!(); From 1050775ac348f9ebe7e3e9d0c7e4875f7a2a1c82 Mon Sep 17 00:00:00 2001 From: Elrendio Date: Thu, 8 Oct 2026 11:55:43 +0200 Subject: [PATCH 2/2] Improve comment --- src/proto/streams/counts.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/proto/streams/counts.rs b/src/proto/streams/counts.rs index ab23d3ecb..f60fd39d6 100644 --- a/src/proto/streams/counts.rs +++ b/src/proto/streams/counts.rs @@ -85,9 +85,8 @@ pub(super) struct Counts { /// connection-level budget for empty, non-final DATA frames. /// - /// Each empty frame is charged the full overhead threshold, while larger - /// non-final frames earn back their payload beyond the threshold, so a - /// peer can only sustain empty frames by also sending payload. + /// Closes the connections upon a burst of empty, non-final DATA frames. + /// Replenishes the budget when non-empty DATA frames are received. empty_data_frame_budget: Budget, }