net/http/httputil: make TestDumpRequest idempotent

TestDumpRequest was failing with -count=2 or more
because for test cases that involved mustReadRequest,
the body was created as a *bufio.Reader. DumpRequest
and DumpRequestOut would then read the body until EOF
and would close it after use.
However, on re-runs of the test, the body would
be terminally exhausted and result in an unexpected
error "http: invalid Read on closed Body".

The update to the test cases adds an extra field "GetReq"
which allows us to construct requests per run of the tests
and hence make the test indefinitely re-runnable/idempotent.
"Req" or "GetReq" are mutually exclusive: either one of them
can be set or nil, but not both.

Fixes #26858

Change-Id: Ice3083dac1aa3249da4afc7075cd984eb159530d
Reviewed-on: https://go-review.googlesource.com/c/153377
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/httputil/dump_test.go b/src/net/http/httputil/dump_test.go
index 63312dd885690ce6a66569dfe3cc166d23c35876..97954ca88d0d1f0f9c1e4f8b95a1222beec22104 100644 (file)
--- a/src/net/http/httputil/dump_test.go
+++ b/src/net/http/httputil/dump_test.go
@@ -18,7 +18,10 @@ import (
 )
 
 type dumpTest struct {
-       Req  http.Request
+       // Either Req or GetReq can be set/nil but not both.
+       Req    *http.Request
+       GetReq func() *http.Request
+
        Body interface{} // optional []byte or func() io.ReadCloser to populate Req.Body
 
        WantDump    string
@@ -29,7 +32,7 @@ type dumpTest struct {
 var dumpTests = []dumpTest{
        // HTTP/1.1 => chunked coding; body; empty trailer
        {
-               Req: http.Request{
+               Req: &http.Request{
                        Method: "GET",
                        URL: &url.URL{
                                Scheme: "http",
@@ -52,7 +55,7 @@ var dumpTests = []dumpTest{
        // Verify that DumpRequest preserves the HTTP version number, doesn't add a Host,
        // and doesn't add a User-Agent.
        {
-               Req: http.Request{
+               Req: &http.Request{
                        Method:     "GET",
                        URL:        mustParseURL("/foo"),
                        ProtoMajor: 1,
@@ -67,7 +70,7 @@ var dumpTests = []dumpTest{
        },
 
        {
-               Req: *mustNewRequest("GET", "http://example.com/foo", nil),
+               Req: mustNewRequest("GET", "http://example.com/foo", nil),
 
                WantDumpOut: "GET /foo HTTP/1.1\r\n" +
                        "Host: example.com\r\n" +
@@ -79,8 +82,7 @@ var dumpTests = []dumpTest{
        // with a bytes.Buffer and hang with all goroutines not
        // runnable.
        {
-               Req: *mustNewRequest("GET", "https://example.com/foo", nil),
-
+               Req: mustNewRequest("GET", "https://example.com/foo", nil),
                WantDumpOut: "GET /foo HTTP/1.1\r\n" +
                        "Host: example.com\r\n" +
                        "User-Agent: Go-http-client/1.1\r\n" +
@@ -89,7 +91,7 @@ var dumpTests = []dumpTest{
 
        // Request with Body, but Dump requested without it.
        {
-               Req: http.Request{
+               Req: &http.Request{
                        Method: "POST",
                        URL: &url.URL{
                                Scheme: "http",
@@ -114,7 +116,7 @@ var dumpTests = []dumpTest{
 
        // Request with Body > 8196 (default buffer size)
        {
-               Req: http.Request{
+               Req: &http.Request{
                        Method: "POST",
                        URL: &url.URL{
                                Scheme: "http",
@@ -145,8 +147,10 @@ var dumpTests = []dumpTest{
        },
 
        {
-               Req: *mustReadRequest("GET http://foo.com/ HTTP/1.1\r\n" +
-                       "User-Agent: blah\r\n\r\n"),
+               GetReq: func() *http.Request {
+                       return mustReadRequest("GET http://foo.com/ HTTP/1.1\r\n" +
+                               "User-Agent: blah\r\n\r\n")
+               },
                NoBody: true,
                WantDump: "GET http://foo.com/ HTTP/1.1\r\n" +
                        "User-Agent: blah\r\n\r\n",
@@ -154,22 +158,25 @@ var dumpTests = []dumpTest{
 
        // Issue #7215. DumpRequest should return the "Content-Length" when set
        {
-               Req: *mustReadRequest("POST /v2/api/?login HTTP/1.1\r\n" +
-                       "Host: passport.myhost.com\r\n" +
-                       "Content-Length: 3\r\n" +
-                       "\r\nkey1=name1&key2=name2"),
+               GetReq: func() *http.Request {
+                       return mustReadRequest("POST /v2/api/?login HTTP/1.1\r\n" +
+                               "Host: passport.myhost.com\r\n" +
+                               "Content-Length: 3\r\n" +
+                               "\r\nkey1=name1&key2=name2")
+               },
                WantDump: "POST /v2/api/?login HTTP/1.1\r\n" +
                        "Host: passport.myhost.com\r\n" +
                        "Content-Length: 3\r\n" +
                        "\r\nkey",
        },
-
        // Issue #7215. DumpRequest should return the "Content-Length" in ReadRequest
        {
-               Req: *mustReadRequest("POST /v2/api/?login HTTP/1.1\r\n" +
-                       "Host: passport.myhost.com\r\n" +
-                       "Content-Length: 0\r\n" +
-                       "\r\nkey1=name1&key2=name2"),
+               GetReq: func() *http.Request {
+                       return mustReadRequest("POST /v2/api/?login HTTP/1.1\r\n" +
+                               "Host: passport.myhost.com\r\n" +
+                               "Content-Length: 0\r\n" +
+                               "\r\nkey1=name1&key2=name2")
+               },
                WantDump: "POST /v2/api/?login HTTP/1.1\r\n" +
                        "Host: passport.myhost.com\r\n" +
                        "Content-Length: 0\r\n\r\n",
@@ -177,9 +184,11 @@ var dumpTests = []dumpTest{
 
        // Issue #7215. DumpRequest should not return the "Content-Length" if unset
        {
-               Req: *mustReadRequest("POST /v2/api/?login HTTP/1.1\r\n" +
-                       "Host: passport.myhost.com\r\n" +
-                       "\r\nkey1=name1&key2=name2"),
+               GetReq: func() *http.Request {
+                       return mustReadRequest("POST /v2/api/?login HTTP/1.1\r\n" +
+                               "Host: passport.myhost.com\r\n" +
+                               "\r\nkey1=name1&key2=name2")
+               },
                WantDump: "POST /v2/api/?login HTTP/1.1\r\n" +
                        "Host: passport.myhost.com\r\n\r\n",
        },
@@ -187,8 +196,7 @@ var dumpTests = []dumpTest{
        // Issue 18506: make drainBody recognize NoBody. Otherwise
        // this was turning into a chunked request.
        {
-               Req: *mustNewRequest("POST", "http://example.com/foo", http.NoBody),
-
+               Req: mustNewRequest("POST", "http://example.com/foo", http.NoBody),
                WantDumpOut: "POST /foo HTTP/1.1\r\n" +
                        "Host: example.com\r\n" +
                        "User-Agent: Go-http-client/1.1\r\n" +
@@ -200,28 +208,40 @@ var dumpTests = []dumpTest{
 func TestDumpRequest(t *testing.T) {
        numg0 := runtime.NumGoroutine()
        for i, tt := range dumpTests {
-               setBody := func() {
-                       if tt.Body == nil {
-                               return
+               if tt.Req != nil && tt.GetReq != nil || tt.Req == nil && tt.GetReq == nil {
+                       t.Errorf("#%d: either .Req(%p) or .GetReq(%p) can be set/nil but not both", i, tt.Req, tt.GetReq)
+                       continue
+               }
+
+               freshReq := func(ti dumpTest) *http.Request {
+                       req := ti.Req
+                       if req == nil {
+                               req = ti.GetReq()
                        }
-                       switch b := tt.Body.(type) {
+
+                       if req.Header == nil {
+                               req.Header = make(http.Header)
+                       }
+
+                       if ti.Body == nil {
+                               return req
+                       }
+                       switch b := ti.Body.(type) {
                        case []byte:
-                               tt.Req.Body = ioutil.NopCloser(bytes.NewReader(b))
+                               req.Body = ioutil.NopCloser(bytes.NewReader(b))
                        case func() io.ReadCloser:
-                               tt.Req.Body = b()
+                               req.Body = b()
                        default:
-                               t.Fatalf("Test %d: unsupported Body of %T", i, tt.Body)
+                               t.Fatalf("Test %d: unsupported Body of %T", i, ti.Body)
                        }
-               }
-               if tt.Req.Header == nil {
-                       tt.Req.Header = make(http.Header)
+                       return req
                }
 
                if tt.WantDump != "" {
-                       setBody()
-                       dump, err := DumpRequest(&tt.Req, !tt.NoBody)
+                       req := freshReq(tt)
+                       dump, err := DumpRequest(req, !tt.NoBody)
                        if err != nil {
-                               t.Errorf("DumpRequest #%d: %s", i, err)
+                               t.Errorf("DumpRequest #%d: %s\nWantDump:\n%s", i, err, tt.WantDump)
                                continue
                        }
                        if string(dump) != tt.WantDump {
@@ -231,8 +251,8 @@ func TestDumpRequest(t *testing.T) {
                }
 
                if tt.WantDumpOut != "" {
-                       setBody()
-                       dump, err := DumpRequestOut(&tt.Req, !tt.NoBody)
+                       req := freshReq(tt)
+                       dump, err := DumpRequestOut(req, !tt.NoBody)
                        if err != nil {
                                t.Errorf("DumpRequestOut #%d: %s", i, err)
                                continue