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.
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.

1 participant