Repository navigation
fix: Honor http.Client.Timeout in roundTripWithOptionalFollowRedirect - #4711
JamBalaya56562 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4711 +/- ##
=======================================
Coverage 98.64% 98.64%
=======================================
Files 204 204
Lines 19080 19164 +84
=======================================
+ Hits 18822 18905 +83
- Misses 258 259 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mangeshraut712
left a comment
There was a problem hiding this comment.
Keeping the deadline until Body.Close() is the right call, and it matches http.Client.Timeout's documented scope ("includes ... reading the response body"). I checked all six callers (downloadArtifactWithoutRateLimit, getArchiveLinkWithoutRateLimit, the workflow run/job log helpers, and repos.go:1428). Each one defer resp.Body.Close()s right after the call, so the wrapper's cancel always runs and no timer outlives the request.
| if t := c.client.Timeout; t > 0 { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(ctx, t) | ||
| defer func() { | ||
| // Callers may still read the body, so keep the deadline until it is closed. | ||
| if resp != nil && resp.Body != nil { | ||
| resp.Body = &cancelOnCloseBody{ReadCloser: resp.Body, cancel: cancel} | ||
| return | ||
| } | ||
| cancel() | ||
| }() | ||
| } |
There was a problem hiding this comment.
Because the 301 branch recurses into this same function, every redirect hop adds another context.WithTimeout layer and wraps the body in another cancelOnCloseBody. That's correct: the inner deadline is min(remaining, t), so the whole chain still shares one budget like http.Client does, and closing the outer wrapper cancels every layer. But the reason isn't obvious from reading it. Moving the timeout setup into a thin entry point and having the 301 branch call an inner, timeout-free helper would make the one-budget-across-redirects semantics explicit and avoid the nesting. Either way, a test with maxRedirects >= 1 where the first hop uses part of the budget would pin that a redirect doesn't reset the timeout.
There was a problem hiding this comment.
Thanks — you're right that each 301 hop nests another context.WithTimeout and cancelOnCloseBody. I added TestRoundTripWithOptionalFollowRedirect_RedirectDoesNotResetTimeout to pin that a redirect doesn't reset the budget (it fails if the inner call gets a fresh timeout: request took 1.6s, want client timeout 1s).
I kept the timeout block at the top of the helper to follow the minimal approach suggested in #4585 (review).
@gmlewis, would you prefer splitting it into an outer entry point and a timeout-free inner helper, or keeping the current shape?
Fixes #4584.
Closes: #4585
Closes: #4688
This takes over #4585, which is blocked on the CLA, and #4688.
roundTripWithOptionalFollowRedirectcalls the transport directly, so thehttp.Client.Timeoutset byWithTimeoutorWithHTTPClientwas never applied.This PR follows the approach suggested in #4585 (review) (the
context.WithTimeoutblock at the top of the helper):http.Client.Dois not used, so transport errors are not wrapped in*url.Error, the cookie jar is not attached, andcheckRedirectHoststill runs first.opts ...RequestOptionparameter is kept.github/github_test.goand usesynctestinstead oftime.Sleep.One difference from the suggested snippet:
defer cancel()cancels the context as soon as the helper returns, butRepositoriesService.GetBranchdecodesresp.Bodyafter the helper returns. With a timeout set, that read fails withread tcp ...: use of closed network connection. So on success the cancel is moved to the response body and runs onClose; on every other exit it runs when the helper returns.Tests:
TestRoundTripWithOptionalFollowRedirect_HonorsHTTPClientTimeout(the test from fix: Honor HTTP client timeouts in redirects #4585): the request ends withcontext.DeadlineExceededafter the client timeout. Fails onmaster(request took 2s, want client timeout 1s).TestRoundTripWithOptionalFollowRedirect_KeepsTimeoutUntilBodyClosed: a 1 MiB body can still be read after the helper returns, and the request context is canceled once the body is closed. Fails onmaster, and also fails with a plaindefer cancel().