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

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

Comments

Projects
None yet
4 participants
@anacrolix
Contributor

anacrolix commented Jan 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 24, 2016

Member

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.

Member

bradfitz commented Jan 24, 2016

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 from Memory leak when using h2 to net/http: http2 Transport retains Request.Body after request is complete, not GCed Jan 24, 2016

@anacrolix

This comment has been minimized.

Show comment
Hide comment
@anacrolix

anacrolix Jan 24, 2016

Contributor

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

Contributor

anacrolix commented Jan 24, 2016

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

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 24, 2016

Member

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

Member

bradfitz commented Jan 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Jan 25, 2016

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

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Jan 25, 2016

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

Contributor

randall77 commented Jan 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 25, 2016

Member

Perfect! That helps a ton.

How did you get that output?

Member

bradfitz commented Jan 25, 2016

Perfect! That helps a ton.

How did you get that output?

@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 Jan 25, 2016

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 .

Contributor

randall77 commented Jan 25, 2016

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

@gopherbot gopherbot closed this in 99fa8c3 Jan 26, 2016

@anacrolix

This comment has been minimized.

Show comment
Hide comment
@anacrolix

anacrolix Jan 26, 2016

Contributor

Great, thanks

Contributor

anacrolix commented Jan 26, 2016

Great, thanks

@anacrolix

This comment has been minimized.

Show comment
Hide comment
@anacrolix

anacrolix Feb 1, 2016

Contributor

This doesn't seem to have fixed it.

Contributor

anacrolix commented Feb 1, 2016

This doesn't seem to have fixed it.

@anacrolix

This comment has been minimized.

Show comment
Hide comment
@anacrolix

anacrolix Feb 1, 2016

Contributor

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.

Contributor

anacrolix commented Feb 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 1, 2016

Member

@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

Member

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

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Feb 1, 2016

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

gopherbot pushed a commit to golang/net that referenced this issue Feb 1, 2016

http2: don't add *Response to activeRes in Transport on Headers.END_S…
…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

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Feb 1, 2016

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

@gopherbot gopherbot closed this in 125f52d Feb 1, 2016

@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.