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: http2 Transport retains Request.Body after request is complete, not GCed #14084

Closed
anacrolix opened this issue Jan 24, 2016 · 13 comments
Closed

net/http: http2 Transport retains Request.Body after request is complete, not GCed #14084

anacrolix opened this issue Jan 24, 2016 · 13 comments
Labels
FrozenDueToAge
Milestone
Go1.6

Comments

@anacrolix
Copy link
Contributor

I believe there's some kind of memory leak when using the following Client. b []byte below is typically 16KB at a time, and is never GC'd after having entered this section of code. Memory use climbs proportional to the number of requests sent on the client. I've ruled out the calling code. It didn't occur before I switched to h2 from HTTP/1.1. I assume some kind of h2 session-related item isn't being cleaned-up due to my unusual configuration.

Client: &http.Client{
                Transport: &http2.Transport{
                    TLSClientConfig: &tls.Config{
                        InsecureSkipVerify: true,
                        NextProtos:         []string{"h2"},
                    },
                },
            },

...

    req, err := http.NewRequest("PATCH", me.url, bytes.NewReader(b))
    if err != nil {
        return
    }
    req.Header.Set("Content-Range", fmt.Sprintf("bytes=%d-", me.off))
    req.ContentLength = int64(len(b))
    resp, err := me.fs.Client.Do(req)
    if err != nil {
        return
    }
    resp.Body.Close()
@bradfitz
Copy link
Contributor

Interesting.

I wrote a test that reproduces this (in https://go-review.googlesource.com/18873) but I can't figure out what's retaining the memory. As far as I can tell, it should be GC'ed.

/cc @aclements @randall77 for help if either of you have time.

@bradfitz bradfitz added this to the Go1.6Maybe milestone Jan 24, 2016
@bradfitz bradfitz changed the title Memory leak when using h2 net/http: http2 Transport retains Request.Body after request is complete, not GCed Jan 24, 2016
@anacrolix
Copy link
Contributor Author

It also occurs on 1.5. I take it it's nontrvial to see what is referencing the bytes ?

@bradfitz
Copy link
Contributor

We have a heap dumper and tools to kinda view said heap but I'm not sure how to really use it these days.

@gopherbot
Copy link

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

@randall77
Copy link
Contributor

It's being held alive because it is in the reqCanceler map. Does that help any?
(in the list below, pointers go from bottom to top).

tracealloc(0xc820114ee0, 0x20, strings.Reader)
tracealloc(0xc820118270, 0x10, ioutil.nopCloser)
tracealloc(0xc8200f1260, 0xe0, http.Request)
tracealloc(0xc8200a42d0, 0x90, map.bucket[_http.Request]func()) (setReqCanceler: t.reqCanceler[r] = fn)
tracealloc(0xc820116900, 0x30, map.hdr[_http.Request]func()) (setReqCanceler: t.reqCanceler = make(map[*Request]func()))
tracealloc(0xc8200e80c0, 0xc0, http.Transport)
tracealloc(0xc82010c480, 0x60) // cst.c

@bradfitz
Copy link
Contributor

Perfect! That helps a ton.

How did you get that output?

@randall77
Copy link
Contributor

I added a print statement to print the address of the strings.Reader.
I ran the test with GODEBUG=allocfreetrace=1 and saved the output.
Then I put a heap dumper in and ran the view on the output.
Then I found the strings.Reader in question using the address printed above.
Then I walked back the object graph using the viewer, using the allocfreetrace dump to find the allocation site (and type) for each object .

@bradfitz bradfitz modified the milestones: Go1.6, Go1.6Maybe Jan 25, 2016
@anacrolix
Copy link
Contributor Author

Great, thanks

@anacrolix
Copy link
Contributor Author

This doesn't seem to have fixed it.

@anacrolix
Copy link
Contributor Author

I had a play with your test. If I make the server response have an empty body, by removing the line
io.WriteString(w, "Hello.")
The h2 test will hang.

@bradfitz bradfitz reopened this Feb 1, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2016

@anacrolix, nice! Confirmed. And that one's a bug in http2, not in the net/http integration. Easy fix: https://go-review.googlesource.com/19134

@gopherbot
Copy link

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

gopherbot pushed a commit to golang/net that referenced this issue Feb 1, 2016
@bradfitz
http2: don't add *Response to activeRes in Transport on Headers.END_S…
644ffc0 
…TREA

Prevents a memory leak.

Tests (to be updated) in Go standard library.

Updates golang/go#14084

Change-Id: I3ff602a013bb8fda7a17bccb31beadb08421ae6a
Reviewed-on: https://go-review.googlesource.com/19134
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz bradfitz self-assigned this Feb 1, 2016
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Feb 3, 2017
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.6
Development

No branches or pull requests
4 participants
@bradfitz @anacrolix @randall77 @gopherbot