fix(generator): a request dropped by the HTTP/2 transport is not retried unless its error has a recognized shape - #129
Merged
Conversation
…ied unless its error has a recognized shape
…along with the caller's deadline
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.
What does this PR do?
isRetryableNetworkErrorin the generatedretry.gonow retries any request that got no response unless the failure is known to be permanent: a canceled context, a certificate the client does not trust, a server that does not speak TLS, an unsupported URL scheme, or a redirect loop. It no longer enumerates transient shapes (EOF sentinels,net.Errortimeouts,net.OpError).An expired deadline is deliberately not excluded. Since Go 1.23 a per-attempt
http.Client.Timeoutsatisfieserrors.Is(err, context.DeadlineExceeded), and that attempt is exactly the one worth repeating. When the caller's own context has expired, the retry loop's wait returns onctx.Done()before sleeping.The new e2e test drives the generated client over a hand-rolled HTTP/2 connection that resets the first stream with
INTERNAL_ERRORand answers the retry, checks that a client timeout is retried for every attempt, and pins the untrusted-certificate and unsupported-scheme cases to a single attempt. The reset and timeout tests each fail against the previous template.Why is this PR needed?
net/http serves HTTP/2 from an internal package and reports a dropped connection through unexported types: a reset stream, "client connection lost", or "cannot retry err after Request.Body was written" when its own retry gives up. None of those is a
net.OpErroror a timeout, so the generated client returned them to the caller on the first attempt even with retries enabled, while a plain TCP reset on HTTP/1 was retried. x/net v0.59.0 deprecated the last exported handle on these errors with no replacement, which settles that inspecting them is not the supported path.Excluding the permanent failures is the pattern go-retryablehttp uses. It is stable because the exclusions are the client's own misconfiguration rather than transport wording. Two of them still have to match text ("unsupported protocol scheme" and "stopped after N redirects") because net/http gives those errors no exported type, and both strings have been unchanged since Go 1.0.
The idempotency guard is unchanged: the typed client still retries only idempotent methods, and the streaming path only when the caller opted in with a replayable body.
A consumer with a hand-written test asserting that an opaque
RoundTrippererror is not retried will need to update it to a permanent error such ascontext.Canceled; core'ssdk/go/raw_test.gohas one.Steps to Reproduce