Skip to content

Re-populate HTTP response body of ErrorResponse - #1363

Merged
gmlewis merged 1 commit into
google:masterfrom
haya14busa:errresp
Jan 8, 2020
Merged

Re-populate HTTP response body of ErrorResponse#1363
gmlewis merged 1 commit into
google:masterfrom
haya14busa:errresp

Conversation

@haya14busa

Copy link
Copy Markdown
Contributor

Fixes #1136

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jan 3, 2020

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @haya14busa!
LGTM.

Awaiting second LGTM before merging.

@gmlewis
gmlewis requested a review from gauntface January 4, 2020 01:21

@wesleimp wesleimp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@gmlewis

gmlewis commented Jan 8, 2020

Copy link
Copy Markdown
Collaborator

Thank you, @wesleimp!
Merging.

@gmlewis
gmlewis merged commit b57bcf6 into google:master Jan 8, 2020
n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
yavorl added a commit to yavorl/go-github that referenced this pull request Aug 25, 2026
`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect error-parsing on failure of creating review

4 participants