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
Closed

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

bgentry opened this issue Nov 19, 2014 · 4 comments

Comments

@bgentry
Copy link
Contributor

bgentry 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
Copy link
Contributor

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
@odeke-em
Copy link
Member

Dup of #8633?

@bradfitz
Copy link
Contributor

Yes, dup.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Apr 1, 2016
…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.
Projects
None yet
Development

No branches or pull requests

6 participants