net/http/httputil: fix goroutine leak for DumpRequestOut

When an invalid URL was passed to DumpRequestOut, it would directly return
without gracefully shutting down the reader goroutine.

So we create a channel and signal the reader goroutine to exit
if an error occurs during roundtrip.

Fixes #32571

Change-Id: I8c2970f1601e599f3d1ebfed298faf5f5716fc2c
Reviewed-on: https://go-review.googlesource.com/c/go/+/182037
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>

diff --git a/src/net/http/httputil/dump.go b/src/net/http/httputil/dump.go
index 7104c3745452ad42853fe00193231812263aa736..81c279515617652e8c4ce8104d5cce001b7cd407 100644 (file)
--- a/src/net/http/httputil/dump.go
+++ b/src/net/http/httputil/dump.go
@@ -111,6 +111,10 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
        }
        defer t.CloseIdleConnections()
 
+       // We need this channel to ensure that the reader
+       // goroutine exits if t.RoundTrip returns an error.
+       // See golang.org/issue/32571.
+       quitReadCh := make(chan struct{})
        // Wait for the request before replying with a dummy response:
        go func() {
                req, err := http.ReadRequest(bufio.NewReader(pr))
@@ -120,13 +124,18 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
                        io.Copy(ioutil.Discard, req.Body)
                        req.Body.Close()
                }
-               dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n")
+               select {
+               case dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n"):
+               case <-quitReadCh:
+               }
        }()
 
        _, err := t.RoundTrip(reqSend)
 
        req.Body = save
        if err != nil {
+               pw.Close()
+               quitReadCh <- struct{}{}
                return nil, err
        }
        dump := buf.Bytes()

diff --git a/src/net/http/httputil/dump_test.go b/src/net/http/httputil/dump_test.go
index 97954ca88d0d1f0f9c1e4f8b95a1222beec22104..85731d36f428c354de19557c37c7ef9108508a3a 100644 (file)
--- a/src/net/http/httputil/dump_test.go
+++ b/src/net/http/httputil/dump_test.go
@@ -26,6 +26,7 @@ type dumpTest struct {
 
        WantDump    string
        WantDumpOut string
+       MustError   bool // if true, the test is expected to throw an error
        NoBody      bool // if true, set DumpRequest{,Out} body to false
 }
 
@@ -206,6 +207,16 @@ var dumpTests = []dumpTest{
 }
 
 func TestDumpRequest(t *testing.T) {
+       // Make a copy of dumpTests and add 10 new cases with an empty URL
+       // to test that no goroutines are leaked. See golang.org/issue/32571.
+       // 10 seems to be a decent number which always triggers the failure.
+       dumpTests := dumpTests[:]
+       for i := 0; i < 10; i++ {
+               dumpTests = append(dumpTests, dumpTest{
+                       Req:       mustNewRequest("GET", "", nil),
+                       MustError: true,
+               })
+       }
        numg0 := runtime.NumGoroutine()
        for i, tt := range dumpTests {
                if tt.Req != nil && tt.GetReq != nil || tt.Req == nil && tt.GetReq == nil {
@@ -250,6 +261,15 @@ func TestDumpRequest(t *testing.T) {
                        }
                }
 
+               if tt.MustError {
+                       req := freshReq(tt)
+                       _, err := DumpRequestOut(req, !tt.NoBody)
+                       if err == nil {
+                               t.Errorf("DumpRequestOut #%d: expected an error, got nil", i)
+                       }
+                       continue
+               }
+
                if tt.WantDumpOut != "" {
                        req := freshReq(tt)
                        dump, err := DumpRequestOut(req, !tt.NoBody)