Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: docs show insufficient way to cleanup Response Body #9134

Closed
bgentry opened this issue Nov 19, 2014 · 4 comments

Comments

Projects
None yet
6 participants
@bgentry
Copy link
Contributor

commented Nov 19, 2014

What does 'go version' print?

go version go1.4rc1 darwin/amd64

What steps reproduce the problem?
If possible, include a link to a program on play.golang.org.

1. View the docs at http://tip.golang.org/pkg/net/http/

What happened?

Docs show the following method for cleaning up response bodies:

resp, err := http.Get("http://example.com/";)
if err != nil {
    // handle error
}
defer resp.Body.Close()

What should have happened instead?

The method shown above does not handle the case where both resp and err are non-nil,
which was preserved for compatibility reasons in #3795. If the response was a redirect
and the CheckRedirect func returned an error, the http.Client will return both the
Response and an error. That response's Body would never be closed under the example
shown above, and its connection would leak.

Unfortunately, I suspect this compatibility fix has probably led to lots of potential
leaks in Go code in the wild. Since breaking Go 1 compatibility is not an option, the
docs for Go 1 should show people how to correctly clean up after responses without
leaking conns & file descriptors.

Instead, the following should work and would not leak:

resp, err := http.Get("http://example.com/";)
if resp != nil {
    defer resp.Body.Close()
}
if err != nil {
    // handle error
}

Please provide any additional information below.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2014

Comment 1:

Labels changed: added repo-main, release-none, documentation.

@bradfitz bradfitz removed the new label Dec 18, 2014

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc removed release-none labels Apr 10, 2015

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 27, 2016

Dup of #8633?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

Yes, dup.

@bradfitz bradfitz closed this Mar 31, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Mar 31, 2016

CL https://golang.org/cl/21364 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 1, 2016

net/http: clean up the Client redirect code, document Body.Close rule…
…s more

Issue #8633 (and #9134) noted that we didn't document the rules about
closing the Response.Body when Client.Do returned both a non-nil
*Response and a non-nil error (which can only happen when the user's
CheckRedirect returns an error).

In the process of investigating, I cleaned this code up a bunch, but
no user-visible behavior should have changed, except perhaps some
better error messages in some cases.

It turns out it's always been the case that when a CheckRedirect error
occurs, the Response.Body is already closed. Document that.

And the new code makes that more obvious too.

Fixes #8633

Change-Id: Ibc40cc786ad7fc4e0cf470d66bb559c3b931684d
Reviewed-on: https://go-review.googlesource.com/21364
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>

@golang golang locked and limited conversation to collaborators Mar 31, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.