http2: take the credit wait before the flush that provokes the credit - #225
Merged
Conversation
A streamed writer that runs out of window flushes what it has staged before parking, because WINDOW_UPDATE is what a peer sends once it has consumed DATA - parking with those bytes still buffered waits for a message the wait itself prevents. That flush is an await, and the WINDOW_UPDATE it exists to provoke can arrive while the writer is still inside it. The writer had not registered as a credit waiter yet, so ReleaseCreditWaiters found nothing for the stream and dropped the wake-up. The writer then registered and parked on a message that had already been and gone, and the response stopped there - the client saw a stall until its own timeout. Registering before the flush closes it: credit arriving during the flush completes that task, and the await after it returns at once. The new test drives the race deterministically. It differs from DrainWithCredit in one respect that is the whole point - it credits while a flush is still in flight rather than after releasing it, which is the window that loses the wake-up. It also credits 8 KiB at a time: a window large enough to carry the rest of the body lets the writer finish without ever parking, and an earlier draft of this test passed against the bug for exactly that reason. Unit 43, E2E 180, Chaos 47, Http 44, File 4 - 0 failed. Claude-Session: https://claude.ai/code/session_01TXm3GLPyPBpiu2bzqZdEHJ
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
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.
The bug
A streamed HTTP/2 writer that runs out of window flushes what it has staged before parking, because WINDOW_UPDATE is what a peer sends once it has consumed DATA — parking with those bytes still buffered waits for a message that the wait itself prevents.
That flush is an
await, and the WINDOW_UPDATE it exists to provoke can arrive while the writer is still inside it:ReleaseCreditWaitersruns during the flush, finds no waiter for the stream, and drops the wake-up. The writer then registers and parks on a message that has already been and gone. The response stops mid-body and the client stalls until its own timeout.Registering before the flush closes it: credit arriving during the flush completes that task, and the await after it returns at once. It is the only call site.
How it showed up
Intermittent 10-second
TaskCanceledExceptiontimeouts in GenHTTP's acceptance suite, on the two tests that push a body past the initial 65535-byte window. Roughly one run in eight, and never in isolation — 22 consecutive runs of those tests alone passed, including pinned to two CPUs.The test
h2 body: credit arriving while the pre-park flush is in flight is not lostdrives it deterministically. Two details matter, both learned the hard way:DrainWithCreditcredits only oncePendingFlushesis 0 — after the flush has been released and the writer is already registered — which is why the existing coverage never saw this.It fails on
main(DATA with END_STREAM for stream 1never arrives) and passes with the fix.Validation
Unit 43, E2E 180, Chaos 47, Http 44, File 4 — 0 failed.