Repository navigation
fix: wait for a stream slot at the peer's limit instead of failing - #972
Open
lennartschoch wants to merge 1 commit into
Open
lennartschoch wants to merge 1 commit into
lennartschoch wants to merge 1 commit into
Conversation
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.
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.
When a shared HTTP/2 connection is at the peer's
SETTINGS_MAX_CONCURRENT_STREAMS,h2answerssend_request_headerswith{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
SocketsHttpHandlerwaits for a stream by default, and Go does the same withStrictMaxConcurrentStreams. 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/2queues 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 inconnected, since no request can be taken before then anyway. The wait is bounded by the connection'sconnect_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-requestcheckout_timeoutoption, and this uses the timeout the connection was dialled with instead. Threading the option throughReqOptsis 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 eunitis green, 1185 tests, xref and dialyzer clean. Four new tests intest/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 afterconnect_timeoutwithcheckout_timeoutwhen 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 advertisesmax_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.