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.

Sign up for GitHub

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

Jump to bottom

net/http: Response.Body should be closed while RoundTripper returned a response & error #7620

Closed
gopherbot opened this issue Mar 24, 2014 · 17 comments
Closed

net/http: Response.Body should be closed while RoundTripper returned a response & error #7620

gopherbot opened this issue Mar 24, 2014 · 17 comments
Labels
FrozenDueToAge
Milestone
Go1.3

Comments

@gopherbot
Copy link

by ucgggg:

What does 'go version' print?
go version go1.2.1 linux/amd64

What steps reproduce the problem?
1. run http://play.golang.org/p/2EVI4La_Fp

What happened?
The memory exhausted quickly.

Please provide any additional information below.
################
net\http\client.go:
################
func send(req *Request, t RoundTripper) (resp *Response, err error) {
       .......
    resp, err = t.RoundTrip(req)
    if err != nil {
        if resp != nil {
            log.Printf("RoundTripper returned a response & error; ignoring response")
        }
        return nil, err
    }
}
################
If "err!=nil", the "resp" will be ignored. But the resp.Body should
be closed if not nil:

if resp != nil {
    log.Printf("RoundTripper returned a response & error; ignoring response")
    if resp.Body != nil {
        resp.Body.Close()
    }
}
@bradfitz
Copy link
Contributor

bradfitz commented Apr 5, 2014

Comment 1:

I suppose we could do this (harmless enough), but it's really a bug in your RoundTripper
implementation.  The log warning is there to tell you that you've implemented
RoundTripper incorrectly.

Labels changed: added release-go1.3maybe, repo-main.

Owner changed to @bradfitz.

Status changed to Accepted.

@jbardin
Copy link
Contributor

jbardin commented Apr 6, 2014

Comment 2:

+bradfitz - good point, but wouldn't that imply that this is bug in http.Transport that
this patch would be hiding? (the repro script above is just using DefaultClien).

@snaury
Copy link
Contributor

snaury commented Apr 6, 2014

Comment 3:

This patch (against release branch) fixes this bug in persistConn (where it actually
happens). If it's ok I'll send a CL against tip.

Attachments:

  1. gzip-roundtripper.patch (591 bytes)

@snaury
Copy link
Contributor

snaury commented Apr 6, 2014

Comment 4:

CL sent: https://golang.org/cl/84850043

@snaury
Copy link
Contributor

snaury commented Apr 6, 2014

Comment 5:

On second thought, maybe this whole gzip reader thing is wrong. Why would persistConn
even look at the gzip header at all? The server has returned a response, so what if the
body is corrupted, it should only matter if user actually tries to read the body, it
shouldn't shadow any actual response and error the server returned.

@snaury
Copy link
Contributor

snaury commented Apr 6, 2014

Comment 6:

I've updated CL to actually call gzip.NewReader lazily, I think it's a lot cleaner that
way.

@gopherbot
Copy link
Author

Comment 7 by ibilicc:

thanks a lot, i think this will solve my problem
https://groups.google.com/forum/#!topic/golang-nuts/FnJZ9iZ0i_g

@gopherbot
Copy link
Author

Comment 8 by ibilicc:

I've updated the patch, and after one night of running my checker, the goroutines now
stop leaking. Big thanks to you all!

@gopherbot
Copy link
Author

Comment 9 by ibilicc:

I've applied the patch, and after one night of running my checker, the goroutines now
stop leaking. Big thanks to you all!

@gopherbot
Copy link
Author

Comment 10:

CL https://golang.org/cl/84850043 references this issue.

@gopherbot
Copy link
Author

Comment 11:

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

@gopherbot
Copy link
Author

Comment 12:

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

@bradfitz
Copy link
Contributor

Comment 13:

This issue was updated by revision 6278a95.

Fixes issue #7569
LGTM=adg
R=golang-codereviews, adg
CC=dsymonds, golang-codereviews, rsc
https://golang.org/cl/86290043

@bradfitz
Copy link
Contributor

Comment 14:

Could you verify that the bug still exists at tip, at or after revision 07e31caba5b6?
Thanks!

@gopherbot
Copy link
Author

Comment 15 by ucgggg:

I tested the script with revision 07e31caba5b6. It now stop leaking. But I don't know
whether 07e31caba5b6 will solve the problem
https://groups.google.com/forum/#!topic/golang-nuts/FnJZ9iZ0i_g.

@jbardin
Copy link
Contributor

jbardin commented Apr 10, 2014

Comment 16:

I can confirm that an issue I had with leaking goroutines when *not* reading a gzipped
Body are now gone too. Thanks!

@bradfitz
Copy link
Contributor

Comment 17:

Fixed as part of https://golang.org/cl/86290043
Related bug now moved to issue #7750.

Status changed to Fixed.

@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.3
Development

No branches or pull requests
5 participants
@bradfitz @jbardin @snaury @rsc @gopherbot