http: fix transport bug with zero-length bodies

An optimization in Transport which re-uses TCP
connections early in the case where there is
no response body interacted poorly with
ErrBodyReadAfterClose.  Upon recycling the TCP
connection early we would Close the Response.Body
(in case the user forgot to), but in the case
of a zero-lengthed body, the user's handler might
not have run yet.

This CL makes sure the Transport doesn't try
to Close requests when we're about to immediately
re-use the TCP connection.

This also includes additional tests I wrote
while debugging.

R=rsc, bradfitzgoog
CC=golang-dev
https://golang.org/cl/4529050

diff --git a/src/pkg/http/serve_test.go b/src/pkg/http/serve_test.go
index f2fb98e3e2de26c03244dc7adcb80c5a9621f962..c9305682d2d0ce5984c95bd2cbbd07ef7074567b 100644 (file)
--- a/src/pkg/http/serve_test.go
+++ b/src/pkg/http/serve_test.go
@@ -710,3 +710,47 @@ func TestRedirectMunging(t *testing.T) {
                t.Errorf("Location header was %q; want %q", g, e)
        }
 }
+
+// TestZeroLengthPostAndResponse exercises an optimization done by the Transport:
+// when there is no body (either because the method doesn't permit a body, or an
+// explicit Content-Length of zero is present), then the transport can re-use the
+// connection immediately. But when it re-uses the connection, it typically closes
+// the previous request's body, which is not optimal for zero-lengthed bodies,
+// as the client would then see http.ErrBodyReadAfterClose and not 0, os.EOF.
+func TestZeroLengthPostAndResponse(t *testing.T) {
+       ts := httptest.NewServer(HandlerFunc(func(rw ResponseWriter, r *Request) {
+               all, err := ioutil.ReadAll(r.Body)
+               if err != nil {
+                       t.Fatalf("handler ReadAll: %v", err)
+               }
+               if len(all) != 0 {
+                       t.Errorf("handler got %d bytes; expected 0", len(all))
+               }
+               rw.Header().Set("Content-Length", "0")
+       }))
+       defer ts.Close()
+
+       req, err := NewRequest("POST", ts.URL, strings.NewReader(""))
+       if err != nil {
+               t.Fatal(err)
+       }
+       req.ContentLength = 0
+
+       var resp [5]*Response
+       for i := range resp {
+               resp[i], err = DefaultClient.Do(req)
+               if err != nil {
+                       t.Fatalf("client post #%d: %v", i, err)
+               }
+       }
+
+       for i := range resp {
+               all, err := ioutil.ReadAll(resp[i].Body)
+               if err != nil {
+                       t.Fatalf("req #%d: client ReadAll: %v", i, err)
+               }
+               if len(all) != 0 {
+                       t.Errorf("req #%d: client got %d bytes; expected 0", i, len(all))
+               }
+       }
+}

diff --git a/src/pkg/http/transport.go b/src/pkg/http/transport.go
index 73a2c2191ea403adec3cdd57f2d9ba6b031a0288..a7b1b20e6305491b579456fc536a806de3e1bea0 100644 (file)
--- a/src/pkg/http/transport.go
+++ b/src/pkg/http/transport.go
@@ -469,6 +469,17 @@ func (pc *persistConn) readLoop() {
                                        waitForBodyRead <- true
                                }
                        } else {
+                               // When there's no response body, we immediately
+                               // reuse the TCP connection (putIdleConn), but
+                               // we need to prevent ClientConn.Read from
+                               // closing the Response.Body on the next
+                               // loop, otherwise it might close the body
+                               // before the client code has had a chance to
+                               // read it (even though it'll just be 0, EOF).
+                               pc.cc.lk.Lock()
+                               pc.cc.lastbody = nil
+                               pc.cc.lk.Unlock()
+
                                pc.t.putIdleConn(pc)
                        }
                }

diff --git a/src/pkg/io/multi_test.go b/src/pkg/io/multi_test.go
index 3ecb7c75d99a67a3d6c506e4e0d54aa379dc1d44..1b3589ddebae8bfe183a53457dde69f0cf6c0a26 100644 (file)
--- a/src/pkg/io/multi_test.go
+++ b/src/pkg/io/multi_test.go
@@ -20,8 +20,9 @@ func TestMultiReader(t *testing.T) {
        nread := 0
        withFooBar := func(tests func()) {
                r1 := strings.NewReader("foo ")
-               r2 := strings.NewReader("bar")
-               mr = MultiReader(r1, r2)
+               r2 := strings.NewReader("")
+               r3 := strings.NewReader("bar")
+               mr = MultiReader(r1, r2, r3)
                buf = make([]byte, 20)
                tests()
        }

diff --git a/src/pkg/mime/multipart/multipart_test.go b/src/pkg/mime/multipart/multipart_test.go
index 8222fbd8a4db43a6b6c1738142a38cb7cf4f3247..a7efc20f2590ade97127f5d9fa64753cf3d61891 100644 (file)
--- a/src/pkg/mime/multipart/multipart_test.go
+++ b/src/pkg/mime/multipart/multipart_test.go
@@ -307,6 +307,29 @@ Oh no, premature EOF!
        }
 }
 
+func TestZeroLengthBody(t *testing.T) {
+       testBody := strings.Replace(`
+This is a multi-part message.  This line is ignored.
+--MyBoundary
+foo: bar
+
+
+--MyBoundary--
+`,"\n", "\r\n", -1)
+       r := NewReader(strings.NewReader(testBody), "MyBoundary")
+       part, err := r.NextPart()
+       if err != nil {
+               t.Fatalf("didn't get a part")
+       }
+       n, err := io.Copy(ioutil.Discard, part)
+       if err != nil {
+               t.Errorf("error reading part: %v", err)
+       }
+       if n != 0 {
+               t.Errorf("read %d bytes; expected 0", n)
+       }
+}
+
 type slowReader struct {
        r io.Reader
 }