Remove unnecessary draining the resp.Body
#847
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
Thanks, @diorahman, but I don't think this is quite correct yet. Please take a look at my comment.
github/github.go
Outdated
| defer func() { | ||
| // Drain up to 512 bytes and close the body to let the Transport reuse the connection | ||
| io.CopyN(ioutil.Discard, resp.Body, 512) | ||
| resp.Body.Close() |
There was a problem hiding this comment.
I believe that we should go back to how this looked prior to the addition of the drain:
e0ff711
Specifically, this should now be:
defer resp.Body.Close()Note that this is an HTTP client, and its documentation says:
The client must close the response body when finished with it:
...<example elided>...
|
This also means that the title of the PR is not correct. The response body does indeed need to be closed. |
The behavior on connection reuse without manually draining resp.Body is introduced in golang/go@18072ad. Fixes google#843
diorahman
changed the title
resp.Body
github/github.go
Outdated
| resp.Body.Close() | ||
| }() | ||
|
|
||
| resp.Body.Close() |
There was a problem hiding this comment.
@diorahman This still needs a correction.
As @gmlewis mentioned, this should be same as how this looked prior to the addition of the drain:
e0ff711
That means, you should defer this statement so that it executes only when the relevant routine returns.
Please change it to:
defer resp.Body.Close()
github/github.go
Outdated
| resp.Body.Close() | ||
| }() | ||
|
|
||
| defer resp.Body.Close() |
There was a problem hiding this comment.
nit: I would like to see a blank line between the defer and the newResponse... since they aren't closely related.
...and while you're add it, I would like to tightly bind the defer to the c.client.Do above, so if you would like to get rid of the blank line above (line 480), that would be fantastic, IMHO.
Thanks, @diorahman!
The behavior on connection reuse is introduced in golang/go@18072ad.
Fixes #843
This change is