http2: port upstream inbound flow control fixes to stop window leaks - #2
http2: port upstream inbound flow control fixes to stop window leaks#2hiroTamada wants to merge 3 commits into
Conversation
transportResponseBody.Read computed the stream receive-window refresh as
unsent = streamFlow - available + bufPipe.Len()
bufPipe.Len() is body data received but not yet consumed by the
application. The amount that is safe to return to the peer is
streamFlow - available - buffered (what golang.org/x/net/http2 computes),
so the buffered term must be subtracted. Adding it over-credits the
stream window by 2x buffered on every refresh.
With a slow reader (proxy relaying to a backpressured client, rate-
limited download), the receive buffer stays large, the advertised stream
window desyncs from the real connection accounting, and a large download
dies partway with:
stream error: stream ID N; FLOW_CONTROL_ERROR
Add a regression test that downloads 48 MiB over an in-process server
through a Transport configured with a Chrome-like 6 MiB stream window
while consuming the body at ~12 MiB/s. It fails on master with
FLOW_CONTROL_ERROR after ~46 MiB and passes with this fix.
Refs bogdanfinn/tls-client#257
So Kernel services can pin this fork via a go.mod replace while the upstream WINDOW_UPDATE fix (bogdanfinn#24) is pending review. Import-path-only change; no code changes beyond the preceding fix.
Port two golang.org/x/net fixes the fork predates: - 7805fdc3 "http2: rewrite inbound flow control tracking" (client side): replace the threshold-based WINDOW_UPDATE refresh with the inflow accounting type. The old Read path refreshed the connection window only once available dropped below half of cc.connFlow, and Close refunded only unread bytes, so bytes read from a body that was then closed were never returned to the peer. Aborted downloads accumulate the stranded deficit until the connection window reaches zero, at which point every later stream on the pooled connection starves with zero bytes: no read can happen, so no refresh can ever fire again. The refresh ceiling was also cc.connFlow while the advertised window is cc.connFlow+65535, permanently stranding the protocol-default slack. inflow credits every consumed byte exactly once and batches wire updates (>=4KiB, or enough to at least double the peer's remaining window). - 9f24bb44 "http2: properly discard data received after request/response body is closed": break the body pipe before refunding unread credit (not after), fail pipe writes after a break instead of silently discarding, and refund connection-level credit for DATA that loses the race with Close. Also the server-side handler-closed-body refund. TestTransportBodyCloseRaceRefundsConnFlow drives 200 aborted-mid-flood downloads against a frame-level server that strictly obeys the client's advertised windows, then asserts a full-body canary request still completes. It wedges on master (canary starves with zero bytes) and passes with this change.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 99252f9. Configure here.
rgarcia
left a comment
There was a problem hiding this comment.
solid port and the two bugs it fixes are real — I verified your close-race test wedges master terminally and passes here. but I don't think this fixes the starvation issue on its own, so it shouldn't replace #1.
the upstream design still only refunds connection credit on body reads. upstream avoids starvation because it advertises a 1 GB connection window against 4 MB streams; with browser-realistic windows (15.66 MB conn / 6 MB stream), three paused-but-open bodies still pin the whole connection. I ran #1's paused-bodies regression test against this branch: it fails exactly like master. the observed failure pattern (open unread bodies accumulating, no closes before each stall, recovery after origin resets) matches paused streams, not the close-race leak — a terminal credit leak wouldn't recover.
cross-test matrix: #1 passes both tests here; this branch passes its own but fails #1's.
suggest merging this first (better accounting, less fork drift), then rebasing #1 on top — buffer-time refund becomes a small diff on the inflow type. one caveat for the rebase: keep #1's half-window refund cadence rather than inflow's 4 KiB min refresh, which would be chattier than any real browser once refunds move to buffer time.
Summary
Ports two
golang.org/x/netinbound flow-control fixes the fork predates. Together they stop pooled HTTP/2 connections from strangling to zero window when response bodies are closed mid-download — the failure mode where every later stream on a connection hangs with zero bytes until the origin resets it after ~5 minutes.What was wrong
Two independent leaks of connection-level flow control credit, both triggered by aborting response bodies:
cc.connFlow, andClose()refunds only unread bytes. Bytes that were read on a stream that then closed before the half-window threshold was crossed were never refunded. Successive aborted downloads accumulate the deficit; once the window hits zero no read can ever happen again, so no refresh can ever fire — a terminal wedge. The refresh ceiling was alsocc.connFlowwhile the advertised window iscc.connFlow + 65535, permanently stranding the protocol-default slack.processDatacheckedcs.didResetundercc.mubut wrote to the body pipe after releasing it, whileClose()refunded buffered bytes and only then broke the pipe. Data written in between was silently discarded by the broken pipe (pipe.Writesucceeded on a broken pipe) and its credit was never returned.What this ports
7805fdc3"http2: rewrite inbound flow control tracking" (client side): theinflowaccounting type. Every consumed byte is credited exactly once; wire updates are batched (≥4 KiB, or enough to at least double the peer's remaining window). Replaces the threshold refresh and the adaptive stream-window logic, both of which are subsumed. Fixes x/net/http2: flow control desync when receiving data for canceled stream golang/go#56558, x/net/http2: WINDOW_UPDATE sent rate too high and can't be configured golang/go#28732.9f24bb44"http2: properly discard data received after request/response body is closed": break the pipe before refunding unread credit, fail pipe writes after a break, refund connection credit for DATA that loses the race withClose, plus the server-side handler-closed-body refund. Fixes x/net/http2: TestTransportReturnsUnusedFlowControlMultipleWrites failures golang/go#57578.Not ported: the server-side half of the inflow rewrite (out of scope; the server keeps the old
flowaccounting plus the 9f24bb44 refund fix).Testing
TestTransportBodyCloseRaceRefundsConnFlow: drives 200 aborted-mid-flood downloads over one pooled connection against a frame-level fake server that strictly obeys the client's advertised windows, then asserts a full-body canary request completes. Wedges on master (canary starves with zero bytes, window ledger decays to 0) and passes with this change, including-race -count=3.TestTransportSlowReaderLargeResponse(the existing large-download regression test) still passes — the inflow accounting also covers the bug it guards../http2suite compared against master: identical failure set (20 pre-existing failures, e.g.TestHeaderOrder3,TestTransportH2c, andTestTransporthangs on both); no new failures. Suite run with-vet=offbecause pre-existing vet errors infhttp_test.goblock the test build.Behavior notes for review
pipe.Writeon a broken pipe now returnserrClosedPipeWriteinstead of silently discarding; both transport and server callers handle it by refunding.Release
This branch is based on the
v0.6.8-kernel.1lineage (master + kernel patches + module rename), so merging is a maintainer taggingv0.6.8-kernel.2from it and bumpinggithub.com/kernel/fhttpin consumers.Note
Medium Risk
HTTP/2 transport flow-control and body-close ordering changed in core connection pooling paths; module rename breaks consumers until they bump imports. SETTINGS/fingerprint surface is unchanged per PR notes.
Overview
Renames the Go module from
github.com/bogdanfinn/fhttptogithub.com/kernel/fhttpand updates imports,go:generatebundle paths, and tests accordingly.The functional work ports upstream
golang.org/x/netHTTP/2 inbound flow-control fixes so pooled connections no longer wedge when response bodies are aborted mid-transfer:inflowtype that credits every consumed byte and batchesWINDOW_UPDATEframes (≥4 KiB or window-doubling), replacing half-window refresh and the fork’s adaptive small-window stream logic on the client transport.transportResponseBody.Closebreaks the body pipe before refunding unread credit;pipe.Writeafter a break returnserrClosedPipeWriteinstead of silently accepting data.processDataenforces connection + stream windows withtakeInflows, refunds credit when DATA loses the race with a closed body, and the server stops treating handler-closed request bodies as a stream error while returning connection-level flow for discarded DATA.Adds
TestTransportSlowReaderLargeResponseandTestTransportBodyCloseRaceRefundsConnFlowto guard WINDOW_UPDATE desync and connection-credit leaks.Reviewed by Cursor Bugbot for commit 99252f9. Bugbot is set up for automated code reviews on this repo. Configure here.