fix(h2): queue onto the stream worker pool only against a parked worker (#520) - #521
Merged
Conversation
…er (#520) globalH2Pool ran H2 stream handlers on GOMAXPROCS*4 workers behind a size*16 buffered channel, and Submit queued whenever the channel had room. An H2 stream handler is not guaranteed to return: an SSE stream, a long poll, or any handler that streams for the life of its client holds its worker until the peer goes away. Once `size` such handlers were running, every later stream landed in the buffer behind workers that would never come back -- and the pool is process-global, so that starved every H2 connection in the binary, not just the one that filled it. Submit now queues only against a parked worker. `idle` is maintained as (workers parked in the receive) - (tasks queued): a worker credits it before blocking, Submit claims a credit before it may queue, and work that finds no credit runs on its own goroutine -- what net/http does for every stream unconditionally. Credits are fungible, so the count stays exact regardless of which worker takes which task. Measured, on a 2-worker pool with 8 concurrent blocking handlers: 2/8 handlers started before, 8/8 after. The threshold is GOMAXPROCS*4 concurrent streaming handlers, which is why the celeris#498 SSE h2c cells pass on the 32-core bench cluster (128 workers) and stall on a 4-vCPU CI runner (16 workers) with 32 clients. Growing the pool instead -- a surplus worker that joins the pool and retires on an idle TTL -- was implemented and measured at 865 ns/op vs 793 for the one-shot goroutine on a dispatch benchmark, while adding a window where a traffic spike is held as parked goroutines. Rejected. The cost of the fix on that same benchmark (darwin/arm64, 10 cores, including CreateStream and executeHandler bookkeeping) is 751 -> 793 ns/op.
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.
Closes #520.
globalH2Poolqueued into asize*16buffered channel whenever the channel had room. An H2 stream handler is not guaranteed to return -- SSE, long poll, anything that streams for the life of its client holds its worker -- so onceGOMAXPROCS*4such handlers were running, every later stream sat in the buffer behind workers that never come back. The pool is process-global, so that starved every H2 connection in the binary.Fix: queue only against a parked worker.
idle= (workers parked in the receive) - (tasks queued); a worker credits it before blocking,Submitclaims a credit before it may queue, and work that finds no credit gets its own goroutine.Measured
TestH2Pool_StreamingHandlersDoNotStarveLaterStreams: 2/8 handlers started before the fix on a 2-worker pool, 8/8 after.TestH2Pool_IdleCreditTracksParkedWorkerspins the invariant and that credits do not leak across a 64-task burst.This is what makes the SSE h2c cells in #517 (celeris#498) pass on a 4-vCPU CI runner: 32 clients against 16 workers.