fix: Close the original response body on non-2xx responses - #4486
Conversation
CheckResponse substitutes r.Body with a re-readable NopCloser copy on error responses, so asserting on resp.Body.Close alone cannot catch a leak of the network body. Wrap the test client's transport and record whether the body it returned is closed. This test fails at this commit; the fix follows in the next one.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
`CheckResponse` substitutes `r.Body` with a re-readable `NopCloser` copy on every non-2xx response (google#1363). Since google#1772, the error-path `defer resp.Body.Close()` in `bareDo` is registered after that substitution, so the deferred close releases the copy and the network body is never closed. `CopilotService.fetchMetricsReport` and `RepositoriesService.downloadReleaseAssetFromURL` have the same pattern. Consequences of the unclosed network body: - With an `http.Client` that has `Timeout` set and a wrapped transport (which includes clients built via `WithAuthToken`), every non-2xx response parks one `net/http.setRequestCancel.func4` goroutine for the remainder of the timeout. Bounded, but it intermittently fails `goleak`-checked test suites downstream. - Error bodies larger than `maxErrorBodySize` are only partially drained, so the connection is additionally lost. The fix captures the network body before calling CheckResponse and closes that instead. The google#1363 behavior (re-readable error bodies) is unchanged: the substitute is left open for callers.
5c52ef3 to
25b7434
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4486 +/- ##
=======================================
Coverage 98.51% 98.51%
=======================================
Files 195 195
Lines 17691 17694 +3
=======================================
+ Hits 17429 17432 +3
Misses 262 262 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| type closeRecorder struct { | ||
| io.ReadCloser | ||
| closed *bool | ||
| } |
There was a problem hiding this comment.
having this round tripper is also a precondition for the bug. Not a necessary precondition for the test because we're asserting that the underlying cause can't happen - Close() not being called.
yavorl
changed the title
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Ah, I see what you are saying, @yavorl.
I would like to cover all possible unclosed bodies, if possible, so that this issue can truly be recorded as solved. What are your thoughts as to what it would take to accomplish this?
LGTM.
The fix is the same, I have to see whether the test pattern translates to the other invocations. Would you like me to try it in this PR or a separate follow-up? |
|
Well, I'm fine either way. If you want to keep it separate in a new PR, that is fine with me too. Thank you, @yavorl! |
|
Done it in the same PR. Added a comment as well to the godoc of |
2 participants
Fixes #4484
CheckResponsesubstitutesr.Bodywith a re-readableNopClosercopy on everynon-2xx response (#1363). Since #1772, the error-path
defer resp.Body.Close()inbareDois registered after that substitution, so the deferred close releases thecopy and the network body is never closed.
CopilotService.fetchMetricsReportandRepositoriesService.downloadReleaseAssetFromURLhave the same pattern.Consequences of the unclosed network body:
http.Clientthat hasTimeoutset and a wrapped transport (whichincludes clients built via
WithAuthToken), every non-2xx response parks onenet/http.setRequestCancel.func4goroutine for the remainder of the timeout.Bounded, but it intermittently fails
goleak-checked test suites downstream.maxErrorBodySizeare only partially drained, so theconnection is additionally lost. 1Mb seems generous so it can be a non-issue.
The fix captures the network body before calling
CheckResponseand closes thatinstead. The #1363 behavior (re-readable error bodies) is
unchanged: the substitute is left open for callers.
The regression test wraps the test client's transport to observe the network
body and asserts it is closed after an error response. It fails on
masterandpasses with this change; the full package test suite passes.
commit 1: a failing regression test
commit 2: the fix; capture the original body & close it
commit 3: similar regression tests for the other 2 call sites
commit 4: fix the 2 remaining callers & update the
CheckResponse()godoc