Repository navigation
fix: send a request a GOAWAY refused again on a fresh connection - #971
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 stream above the GOAWAY's last_stream_id was never processed (RFC 9113 6.8), so request/5 dials again and sends the request once more whatever its method, as long as the body can be sent twice: a fun body may have been consumed. Once only: a refusal from the fresh connection reaches the caller. An unpooled connection the GOAWAY refused is stopped first; a pooled one has already left its pool.
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.
Follow-up to #961 and #962. Those made a GOAWAY fail only the streams the server refused, and return
{error, {goaway, _}}for them. That error is a guarantee: RFC 9113 §6.8 says a stream abovelast_stream_idwas not processed. So far hackney handed it to the caller, who then has to work out that this particular error is safe to retry, and most callers don't. We saw it as payment requests failing a booking when Stripe recycled a connection, with the charge never having happened.Every other client I looked at retries this itself: Go's http2 transport aborts the stream with an error whose comment reads "indicates that the request should be retried on a new connection", .NET has
RetryOnConnectionFailurefor exactly "server shutting down (GOAWAY)", the JDK marks the streams abovelast_stream_idas unprocessed for automatic retry, libcurl setsrefused_streamand re-runs the transfer on a fresh connection, and OkHttp recovers throughRetryAndFollowUpInterceptor. Go, .NET and curl do it for any method, since the protocol guarantees the request wasn't processed. hackney 1.x did the same kind of transparent reconnect for a stale pooled connection.This does the same in
request/5. When the first attempt comes back as{error, {goaway, _}}, it dials again and sends the request once more, whatever the method. Once only: a refusal from the fresh connection goes to the caller. The one body that can't be sent twice is a fun, which the first attempt may have consumed, so those are returned as before. An unpooled connection the GOAWAY refused is stopped first, since it belongs to the caller who is about to replace it; a pooled one has already left its pool.Not covered on purpose:
send_request/2on a connection fromconnect/4, where the caller holds the connection and a retry would have to swap it under them, and a streamed request body refused after its chunks started, whichsend_body/2reports and the caller has to redo. Both match what the other clients do.rebar3 eunitis green, 1183 tests, xref and dialyzer clean. The drain tests that asserted the refusal now assert the retried result, and three new ones cover a refused stream being sent again on a fresh connection, the same for an unpooled connection, and the refusal from the fresh connection reaching the caller rather than looping. All fail without the change. The test server gained afirst_streamtrigger and atrigger_onoption so a server can refuse on every connection.