Skip to content

fix: wait for a stream slot at the peer's limit instead of failing - #972

Open
lennartschoch wants to merge 1 commit into
benoitc:masterfrom
lennartschoch:fix/h2-wait-for-stream
Open

lennartschoch wants to merge 1 commit into
benoitc:masterfrom
lennartschoch:fix/h2-wait-for-stream

Conversation

@lennartschoch

Copy link
Copy Markdown
Contributor

When a shared HTTP/2 connection is at the peer's SETTINGS_MAX_CONCURRENT_STREAMS, h2 answers send_request_headers with {error, max_streams_exceeded} and hackney passes that straight to the caller. Stripe advertises 128, Braintree 100, so a burst past that fails requests instantly, where the HTTP/1.1 pool would have made the extra callers wait for a free connection.

Nobody else fails here. .NET's SocketsHttpHandler waits for a stream by default, and Go does the same with StrictMaxConcurrentStreams. Go's default, Java, libcurl and OkHttp open another connection instead, which RFC 9113 §9.1 asks clients not to do, and which .NET's docs call out for that reason. Waiting is the smaller change and the one the RFC prefers, so that's what this does; a second connection can stay a separate, opt-in change if someone needs it.

A request that hits the limit is parked on the connection as the call it came in as. When a stream ends, h2_stream_result/2 queues an internal event and the request that has waited longest is re-issued with {next_event, {call, From}, Event}, so it goes back through the same clause it first arrived at, pre-send GOAWAY check and per-request options included. A slot that frees while a request body is being sent is handed out once the connection is back in connected, since no request can be taken before then anyway. The wait is bounded by the connection's connect_timeout, the bound a pool checkout has by default, and runs out with the same {error, checkout_timeout}. One shortcut there: the pool honours a per-request checkout_timeout option, and this uses the timeout the connection was dialled with instead. Threading the option through ReqOpts is a small follow-up if you want the two to match exactly. A GOAWAY, a close, or the idle timer fail the waiting requests too, with the goaway or closed reason: nothing of theirs was sent, so a GOAWAY refusal is the retryable kind.

rebar3 eunit is green, 1185 tests, xref and dialyzer clean. Four new tests in test/hackney_http2_stream_limit_tests.erl, all failing without the change: a request waiting for a stream to end and then going out, a request giving up after connect_timeout with checkout_timeout when the stream never ends, a GOAWAY refusing the waiting request while the accepted stream finishes, and a slot that frees during a streamed request body being used once that body is done. The test server advertises max_streams, can delay its answers, and gained a delayed GOAWAY trigger.

Independent of #971, but both edit the same test helper, test/hackney_h2_goaway_server.erl, so whichever lands second needs a trivial rebase.

A request that finds the shared HTTP/2 connection at the peer's
max_concurrent_streams came back as {error, max_streams_exceeded}. It now
waits for a stream to end and goes out then, for at most connect_timeout,
the bound a pool checkout has, and runs out with the same checkout_timeout.
A slot freed while a request body is being sent is handed out once the
connection takes requests again. A GOAWAY, a close or the idle timer fail the
waiting requests with that reason: nothing of theirs was sent.
@benoitc

benoitc commented Oct 23, 2026

Copy link
Copy Markdown
Owner

The changes look good overall, and tests pass on current master. Before it can be emerged can you look at the following:

  1. When a queued request hits max_streams_exceeded again, it goes to the back with a new timer. Could we keep its original deadline and position instead?
  2. Only one waiter wakes up when a stream becomes available. If it fails without opening a stream, the next one may timeout despite free slots. Maybe we could wake the next waiter, or one per available slot.
  3. It would be good to monitor waiting callers and remove requests when their caller dies.
  4. Could we also respect the per-request checkout_timeout, since that's the error being returned?
  5. The change in behavior could use a mention in guides/http2_guide.md, as requests now wait up to connect_timeout instead of failing immediately with max_streams_exceeded.

The tests use 300–1000 ms sleeps, which might be flaky on CI. Explicit signals would probably be more reliable.

Can you also look to the following when rebasing on #971. Both changes use first_stream for different purposes, and goaway_fails_the_waiting_request will now return {ok, 200, <<"1">>} since #971 retries the request.

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.

2 participants