fix(http2): do not reserve capacity for idle Upgraded streams - #4198
Open
Dev-next-gen wants to merge 1 commit into
Open
fix(http2): do not reserve capacity for idle Upgraded streams#4198Dev-next-gen wants to merge 1 commit into
Dev-next-gen wants to merge 1 commit into
Conversation
The send task of an HTTP/2 `Upgraded` stream reserved one byte of capacity at the top of every loop, before any write was queued. As with `PipeToSendStream` before hyperium#4061, an idle tunnel then pins the last byte of the connection-level window, and a second stream can deadlock against peers that only send WINDOW_UPDATE once their window is exhausted. Pull the next write from the channel first, hold it until h2 has capacity, and only then reserve. The next write is still not pulled before the held one is handed to h2, so the writer keeps seeing backpressure. The task now always polls the channel when it holds nothing, which makes the separate close notification unnecessary, so remove it. Refs hyperium#4003
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reading the fix for #4003 (#4061), I noticed that
UpgradedSendStreamTask::tickinsrc/proto/h2/upgrade.rsstill starts every loop withreserve_capacity(1), before it knows whether anything was written to the tunnel. So an idle HTTP/2 CONNECT tunnel pins one byte of the connection window, the same way an idle request body did before #4061.I added
h2_idle_upgraded_does_not_pin_connection_windowtotests/client.rs, modelled on the #4061 test: a tunnel writes 65534 bytes and then stays open, against a raw h2 server that never releases capacity. On master the one-byte request B never reaches the server and the test times out after 5 seconds. With this change it passes.The task now takes the next write from the channel, keeps it in a
bufferedslot, reserves capacity, waits for it, and then hands the write to h2. That is the same shape asPipeToSendStream. It doesn't pull another write until the buffered one has gone to h2, so the backpressure from #4102 is kept. One difference: since one write sits in the slot instead of the channel, the writer gets one morepoll_writeaccepted before it blocks. Because the task always polls the receiver when it holds nothing, it sees the channel close by itself, so I removedUpgradedCloseNotify, which only served the no-capacity branch.None of this reaches the public API. Everything touched lives in
proto::h2::upgrade, which ispub(crate).UpgradedSendStreamTaskdoes show up in the sealed executor bound inrt/bounds.rs, but its name, generics and auto traits don't change: the removedArc<UpgradedCloseNotify>and the addedOption<Cursor<Box<[u8]>>>are bothSend + Sync + Unpin.cargo test --features fullpasses, including the CONNECT backpressure tests from #4102, and so doescargo checkof the http2 client and server feature combinations with-D dead_code -D unused_imports. My local clippy (1.94) doesn't know a few lints listed in Cargo.toml, so I rancargo clippy --features full -- -D warnings -A unknown_lints, which reports nothing new.AI tools used