net/http: don't retain *http.Request in Transport's HTTP/2 path

Fixes #14084

Change-Id: Icbef5678ab3c4fd7eed2693006c47aca6d831d90
Reviewed-on: https://go-review.googlesource.com/18873
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go
index 9c1aa7920e347c5b0815a8fea5f631ca6dd1e19a..9b581e73117cc4caf06e6777a722e2dcb4525ae6 100644 (file)
--- a/src/net/http/clientserver_test.go
+++ b/src/net/http/clientserver_test.go
@@ -20,6 +20,7 @@ import (
        "net/url"
        "os"
        "reflect"
+       "runtime"
        "sort"
        "strings"
        "sync"
@@ -999,6 +1000,47 @@ func TestTransportDiscardsUnneededConns(t *testing.T) {
        t.Errorf("%d connections opened, %d closed; want %d to close", open, close, open-1)
 }
 
+// tests that Transport doesn't retain a pointer to the provided request.
+func TestTransportGCRequest_h1(t *testing.T) { testTransportGCRequest(t, h1Mode) }
+func TestTransportGCRequest_h2(t *testing.T) { testTransportGCRequest(t, h2Mode) }
+func testTransportGCRequest(t *testing.T, h2 bool) {
+       defer afterTest(t)
+       cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
+               ioutil.ReadAll(r.Body)
+               io.WriteString(w, "Hello.")
+       }))
+       defer cst.close()
+
+       didGC := make(chan struct{})
+       (func() {
+               body := strings.NewReader("some body")
+               req, _ := NewRequest("POST", cst.ts.URL, body)
+               runtime.SetFinalizer(req, func(*Request) { close(didGC) })
+               res, err := cst.c.Do(req)
+               if err != nil {
+                       t.Fatal(err)
+               }
+               if _, err := ioutil.ReadAll(res.Body); err != nil {
+                       t.Fatal(err)
+               }
+               if err := res.Body.Close(); err != nil {
+                       t.Fatal(err)
+               }
+       })()
+       timeout := time.NewTimer(5 * time.Second)
+       defer timeout.Stop()
+       for {
+               select {
+               case <-didGC:
+                       return
+               case <-time.After(100 * time.Millisecond):
+                       runtime.GC()
+               case <-timeout.C:
+                       t.Fatal("never saw GC of request")
+               }
+       }
+}
+
 type noteCloseConn struct {
        net.Conn
        closeFunc func()

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index fc0ae36b519d4810bb135f383216477541b02528..41df906cf2d1d70d3fa1d714ebc4b706f0c692ef 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -298,6 +298,7 @@ func (t *Transport) RoundTrip(req *Request) (*Response, error) {
                var resp *Response
                if pconn.alt != nil {
                        // HTTP/2 path.
+                       t.setReqCanceler(req, nil) // not cancelable with CancelRequest
                        resp, err = pconn.alt.RoundTrip(req)
                } else {
                        resp, err = pconn.roundTrip(treq)
@@ -397,7 +398,8 @@ func (t *Transport) CloseIdleConnections() {
 // CancelRequest cancels an in-flight request by closing its connection.
 // CancelRequest should only be called after RoundTrip has returned.
 //
-// Deprecated: Use Request.Cancel instead.
+// Deprecated: Use Request.Cancel instead. CancelRequest can not cancel
+// HTTP/2 requests.
 func (t *Transport) CancelRequest(req *Request) {
        t.reqMu.Lock()
        cancel := t.reqCanceler[req]