Skip to content

http2: a buffered response stays inside the peer's stream window and waits for credit instead of being reset - #379

Open
MDA2AV wants to merge 1 commit into
mainfrom
fix/353-h2-buffered-stream-window
Open

MDA2AV wants to merge 1 commit into
mainfrom
fix/353-h2-buffered-stream-window

Conversation

@MDA2AV

@MDA2AV MDA2AV commented Oct 17, 2026 •

Copy link
Copy Markdown
Owner

Fixes #353.

What was wrong. RunBufferedAsync wrote a response's DATA bounded by the connection window only.

  • Stream window ignored. A peer whose stream window was smaller than its connection window received more on the stream than it had allowed, which RFC 9113 6.9.1 lets it answer with FLOW_CONTROL_ERROR. The buffered path had nowhere to keep that window. A request that arrives whole leaves _streams before its handler runs, which takes its PendingRequest.SendWindow out of the write's reach, and any WINDOW_UPDATE for the stream after that point was dropped.
  • Reset on exhaustion. When the connection window ran out, the rest of the body was dropped and the stream reset with FLOW_CONTROL_ERROR. Every buffered response larger than the peer's connection window failed, including against a peer that keeps the RFC's default of 65,535 bytes.

What changed. The buffered path now uses the flow control the streamed path already has, instead of its own:

  • The window. A stream's send window moves into _responseWindows as soon as its request is ready: in TryComplete, and in BeginStreamedBody when request bodies are streamed. It carries over the PendingRequest.SendWindow the stream had. WINDOW_UPDATE, SETTINGS, RST_STREAM and teardown already maintain that store.
    • TryDispatchStreamed no longer re-seeds the window with _peerInitialStreamWindow. That also stops streamed responses throwing away credit the peer sent while the request was still arriving.
  • The write. WriteData bounds each frame by the frame size and by both windows. When either window is spent, the rest of the body is copied into a pooled Http2ResponseWriter. The writer sends it as credit arrives, ends the stream the way a streamed response's CompleteAsync does, and goes back to the pool. A single internal method, Http2ResponseWriter.ContinueBody, tells the writer the headers have already gone out.
  • Why a copy. Http2Response lets a handler pool or reuse its body once it returns, and the request's own arena is returned straight after WriteResponse.
  • Reset streams. If the peer reset the stream while its handler ran, the response is no longer sent at all (RFC 9113 5.1). Before, its HEADERS and DATA went out on the closed stream.

No collection or counter is added. A parked response lives in _responseWindows, _creditWaiters and _writerPool, all of which the streamed path already has. A response that fits its windows is still written synchronously, with nothing allocated.

Proof.

  • Promoted from pending: Unit h2 flow: a buffered response waits on the peer's stream window instead of overrunning it.
  • New tests in Http2BufferedCreditTests. This is a sibling file, because Http2FlowControlTests already holds 14 tests; it reuses that file's frame helpers, which are now internal.
    1. h2 flow: a buffered response past the connection window waits for a WINDOW_UPDATE instead of being reset
    2. h2 flow: stream credit sent after a request, before its answer, still counts
    3. h2 flow: a parked buffered response sends the body it was given, not what its buffer holds later
    4. h2 flow: a parked buffered response stops at the peer's RST_STREAM, and the connection serves on
    5. h2 flow: a parked buffered response lets go when the peer sends GOAWAY or closes the connection

Run both ways:

  • Without the fix (tests kept, source reverted to main):
    • The promoted test fails: 65536 bytes of DATA against a 16384-byte stream window, which the peer may answer with FLOW_CONTROL_ERROR.
    • Test 1 fails: the response was reset when the connection window ran out, instead of waiting for credit.
    • Tests 3, 4 and 5 fail at their precondition: expected [16384], got [65536].
    • Test 2 passes on main, which ignores the stream window entirely. It is there to guard this fix.
  • Against a broken version of the fix, to show each new test catches its mistake:
  • With the fix: all pass.

Suites (main 71aab91 in parentheses):

  • Unit: 102 passed, 0 failed, 0 pending (96 passed, 1 pending)
  • Chaos: 79 passed, 0 failed, 1 pending (same; the pending one is http2: DATA on an idle stream is accepted instead of a PROTOCOL_ERROR #354)
  • E2E: 292 passed, 0 failed, 5 pending (same)
  • Http: 88 passed, 0 failed (same)
  • Tls: 169 passed, 0 failed, 14 skipped (same; the kTLS cases need modprobe tls)
  • ioxide.http2 builds for net10.0 and net11.0 with no new warnings. The two CA2014 warnings in Http2Connection.Write.cs are on main too.

Bench. bench/any.sh Http2/ManagedBuffered (h2load, 2 reactors pinned to CPUs 2 and 6, the driver to 0-1 and 12-15). Five rounds alternate main 71aab91, the whole batch merged (for h2, this PR and #378), and main again as the A/A control:

  • main: 4.098M req/s mean, 0.484 µs CPU/req
  • main again: 4.104M (+0.1%), 0.484 µs
  • batch: 4.062M (-0.9%), 0.490 µs (+1.2%)

Single runs range from 3.83M to 4.37M, so -0.9% is inside the noise. Any real part of it would be the one _responseWindows insert and remove each buffered response now makes.

Overlap. #378 (issue #354) changes one condition in HandleData in the same file. The two branches merge cleanly (git merge-tree). On the merged tree, Unit is 102 passed and Chaos 80 passed, both with 0 failed and 0 pending.

All eight together. This batch's PRs (#376 to #383) were merged onto main 71aab91 in one scratch tree. The only conflict is the binary libioxide_ngtcp2.so between #376 and #377, rebuilt there from the merged shim with the script's defaults. Everything builds, and Unit 102, E2E 309, Http 88, Chaos 80, Tls 169 (14 kTLS cases skipped) and File 5 pass, with 0 failed and 0 pending. The repo's only pending test left is the Redis suite's (#336).

Noticed, not changed.

  • Unread responses now hold memory. A peer that never sends credit holds a parked copy of each response's unsent body until it does, or until the connection ends. Before this change those responses were reset. The streamed path already behaves this way.
    • MaxConcurrentStreams does not bound it, because it counts only streams still arriving and not responses being written. That is the known gap to fold into the planned http2 refactor.
  • Dead branch. SendStreamedData still has a branch that charges PendingRequest.SendWindow. With every response window in _responseWindows it can no longer be reached. It is left for that refactor.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http2: a buffered response is bounded by the connection window only, and can overrun the peer's stream window

1 participant