net/http/httputil: fix deadlock in DumpRequestOut

Fix a deadlock in DumpRequestOut which can occur if the request is
cancelled between response being sent and it being processed.

Also:
* Ensure we don't get a reader leak when an error is reported by the
  transport before the body is consumed.
* Add leaked goroutine retries to avoid false test failures.

Fixes #38352

Change-Id: I83710791b2985b997f61fe5b49eadee0bb51bdee
Reviewed-on: https://go-review.googlesource.com/c/go/+/232798
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Damien Neil <dneil@google.com>

diff --git a/src/net/http/httputil/dump.go b/src/net/http/httputil/dump.go
index 4c9d28bed8faffeeb99620ce6a074da5033a12fc..2948f27e5d5c58b4b93a7b3452b56b7062264406 100644 (file)
--- a/src/net/http/httputil/dump.go
+++ b/src/net/http/httputil/dump.go
@@ -138,6 +138,8 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
                select {
                case dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n"):
                case <-quitReadCh:
+                       // Ensure delegateReader.Read doesn't block forever if we get an error.
+                       close(dr.c)
                }
        }()
 
@@ -146,7 +148,8 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
        req.Body = save
        if err != nil {
                pw.Close()
-               quitReadCh <- struct{}{}
+               dr.err = err
+               close(quitReadCh)
                return nil, err
        }
        dump := buf.Bytes()
@@ -167,13 +170,17 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
 // delegateReader is a reader that delegates to another reader,
 // once it arrives on a channel.
 type delegateReader struct {
-       c chan io.Reader
-       r io.Reader // nil until received from c
+       c   chan io.Reader
+       err error     // only used if r is nil and c is closed.
+       r   io.Reader // nil until received from c
 }
 
 func (r *delegateReader) Read(p []byte) (int, error) {
        if r.r == nil {
-               r.r = <-r.c
+               var ok bool
+               if r.r, ok = <-r.c; !ok {
+                       return 0, r.err
+               }
        }
        return r.r.Read(p)
 }

diff --git a/src/net/http/httputil/dump_test.go b/src/net/http/httputil/dump_test.go
index 7571eb0820d33254071994ca616df9cb07c771bb..8168b2ebc05d0d633a43226f37e5e7d03b51fb4e 100644 (file)
--- a/src/net/http/httputil/dump_test.go
+++ b/src/net/http/httputil/dump_test.go
@@ -7,13 +7,17 @@ package httputil
 import (
        "bufio"
        "bytes"
+       "context"
        "fmt"
        "io"
+       "math/rand"
        "net/http"
        "net/url"
        "runtime"
+       "runtime/pprof"
        "strings"
        "testing"
+       "time"
 )
 
 type eofReader struct{}
@@ -311,11 +315,39 @@ func TestDumpRequest(t *testing.T) {
                        }
                }
        }
-       if dg := runtime.NumGoroutine() - numg0; dg > 4 {
-               buf := make([]byte, 4096)
-               buf = buf[:runtime.Stack(buf, true)]
-               t.Errorf("Unexpectedly large number of new goroutines: %d new: %s", dg, buf)
+
+       // Validate we haven't leaked any goroutines.
+       var dg int
+       dl := deadline(t, 5*time.Second, time.Second)
+       for time.Now().Before(dl) {
+               if dg = runtime.NumGoroutine() - numg0; dg <= 4 {
+                       // No unexpected goroutines.
+                       return
+               }
+
+               // Allow goroutines to schedule and die off.
+               runtime.Gosched()
+       }
+
+       buf := make([]byte, 4096)
+       buf = buf[:runtime.Stack(buf, true)]
+       t.Errorf("Unexpectedly large number of new goroutines: %d new: %s", dg, buf)
+}
+
+// deadline returns the time which is needed before t.Deadline()
+// if one is configured and it is s greater than needed in the future,
+// otherwise defaultDelay from the current time.
+func deadline(t *testing.T, defaultDelay, needed time.Duration) time.Time {
+       if dl, ok := t.Deadline(); ok {
+               if dl = dl.Add(-needed); dl.After(time.Now()) {
+                       // Allow an arbitrarily long delay.
+                       return dl
+               }
        }
+
+       // No deadline configured or its closer than needed from now
+       // so just use the default.
+       return time.Now().Add(defaultDelay)
 }
 
 func chunk(s string) string {
@@ -445,3 +477,43 @@ func TestDumpResponse(t *testing.T) {
                }
        }
 }
+
+// Issue 38352: Check for deadlock on cancelled requests.
+func TestDumpRequestOutIssue38352(t *testing.T) {
+       if testing.Short() {
+               return
+       }
+       t.Parallel()
+
+       timeout := 10 * time.Second
+       if deadline, ok := t.Deadline(); ok {
+               timeout = time.Until(deadline)
+               timeout -= time.Second * 2 // Leave 2 seconds to report failures.
+       }
+       for i := 0; i < 1000; i++ {
+               delay := time.Duration(rand.Intn(5)) * time.Millisecond
+               ctx, cancel := context.WithTimeout(context.Background(), delay)
+               defer cancel()
+
+               r := bytes.NewBuffer(make([]byte, 10000))
+               req, err := http.NewRequestWithContext(ctx, http.MethodPost, "http://example.com", r)
+               if err != nil {
+                       t.Fatal(err)
+               }
+
+               out := make(chan error)
+               go func() {
+                       _, err = DumpRequestOut(req, true)
+                       out <- err
+               }()
+
+               select {
+               case <-out:
+               case <-time.After(timeout):
+                       b := &bytes.Buffer{}
+                       fmt.Fprintf(b, "deadlock detected on iteration %d after %s with delay: %v\n", i, timeout, delay)
+                       pprof.Lookup("goroutine").WriteTo(b, 1)
+                       t.Fatal(b.String())
+               }
+       }
+}