Skip to content

fix: send a request a GOAWAY refused again on a fresh connection - #971

Open
lennartschoch wants to merge 1 commit into
benoitc:masterfrom
lennartschoch:fix/h2-retry-refused-request
Open

lennartschoch wants to merge 1 commit into
benoitc:masterfrom
lennartschoch:fix/h2-retry-refused-request

Conversation

@lennartschoch

Copy link
Copy Markdown
Contributor

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 above last_stream_id was 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 RetryOnConnectionFailure for exactly "server shutting down (GOAWAY)", the JDK marks the streams above last_stream_id as unprocessed for automatic retry, libcurl sets refused_stream and re-runs the transfer on a fresh connection, and OkHttp recovers through RetryAndFollowUpInterceptor. 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/2 on a connection from connect/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, which send_body/2 reports and the caller has to redo. Both match what the other clients do.

rebar3 eunit is 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 a first_stream trigger and a trigger_on option so a server can refuse on every connection.

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