Fix connection window credit loss in HTTP/2 backlog accounting - #1032
Open
note89 wants to merge 1 commit into
Open
Fix connection window credit loss in HTTP/2 backlog accounting#1032note89 wants to merge 1 commit into
note89 wants to merge 1 commit into
Conversation
backLogSize was incremented when a stream added a reservation to the backlog, but never reduced when allocate() granted connection window capacity to a backlogged stream. Only the full-clear branch of releaseBackLog() reset it, so after any partial allocation the aggregate stayed larger than the sum of the outstanding per-stream requests. The stale aggregate forced later releaseBackLog() calls down the partial-allocation path even when the WINDOW_UPDATE covered every outstanding request. That path discards any surplus instead of returning it to the connection flow control window, so the server's view of the window ended up permanently smaller than the credit the client had granted. Streams created afterwards could stall waiting for window capacity the client believed it had already provided, until the write timeout closed the connection with ENHANCE_YOUR_CALM. Decrement backLogSize as allocations are made so it always equals the sum of the outstanding connection allocation requests, and extend the RFC 9218 priority test to assert that surplus window update credit remains usable by a subsequent stream. Also fix a digit transposition in an existing comment (5641 -> 5461).
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.
backLogSizeis incremented when streams join the backlog, butallocate()never decrements it when granting connection window capacity to a backlogged stream. Only the full-clear branch ofreleaseBackLog()resets it. After any partial allocation the aggregate is larger than the sum of the outstanding per-stream requests.A later
WINDOW_UPDATEthat covers all outstanding requests is then compared against the stale aggregate, takes the partial-allocation branch, and the surplus is discarded instead of being returned to the connection flow control window. Subsequent streams stall waiting for credit the client has already sent, until the write timeout closes the connection with ENHANCE_YOUR_CALM.In the extended
TestRfc9218scenario: 10,239 bytes of real backlog, a 16,384-byte update — the 6,145-byte surplus is lost and the next stream receives headers but no data.This is a regression from 169b813 ("Switch HTTP/2 to use RFC 9218 priorities rather than RFC 7540", shipped in 10.1.10 / 9.0.76 / 11.0.0-M6). The previous RFC 7540 allocation path decremented the aggregate (
backLogSize -= stream.getConnectionAllocationMade()/...Requested()); the rewritten urgency-based path did not carry that over.Fix: decrement
backLogSizeas allocations are made, keeping it equal to the sum of outstanding requests. The invariant is documented on the field. Also fixes a digit transposition in an existing test comment (5641 -> 5461).Testing: the extended
TestRfc9218fails without the fix (next stream starves until read timeout) and passes with it, asserting the exact 6,145-byte surplus plus a 2,047-byte update completing the 8,192-byte body. Fullorg.apache.coyote.http2package passes. The same issue exists on 9.0.x and 10.1.x.