net/http: reuse client connections earlier when Content-Length is set

Set EOF on the final Read of a body with a Content-Length, which
will cause clients to recycle their connection immediately upon
the final Read, rather than waiting for another Read or Close
(neither of which might come).  This happens often when client
code is simply something like:

  err := json.NewDecoder(resp.Body).Decode(&dest)
  ...

Then there's usually no subsequent Read. Even if the client
calls Close (which they should): in Go 1.1, the body was
slurped to EOF, but in Go 1.2, that was then treated as a
Close-before-EOF and the underlying connection was closed.
But that's assuming the user even calls Close. Many don't.
Reading to EOF also causes a connection be reused. Now the EOF
arrives earlier.

This CL only addresses the Content-Length case. A future CL
will address the chunked case.

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/49570044

diff --git a/src/pkg/net/http/transfer.go b/src/pkg/net/http/transfer.go
index 344b1ba2428a92bc873c9766e1874485c424a78f..2eec9d9abc4788357d636a56d6b5deba79ae6f6b 100644 (file)
--- a/src/pkg/net/http/transfer.go
+++ b/src/pkg/net/http/transfer.go
@@ -559,6 +559,17 @@ func (b *body) readLocked(p []byte) (n int, err error) {
                }
        }
 
+       // If we can return an EOF here along with the read data, do
+       // so. This is optional per the io.Reader contract, but doing
+       // so helps the HTTP transport code recycle its connection
+       // earlier (since it will see this EOF itself), even if the
+       // client doesn't do future reads or Close.
+       if err == nil && n > 0 {
+               if lr, ok := b.src.(*io.LimitedReader); ok && lr.N == 0 {
+                       err = io.EOF
+               }
+       }
+
        return n, err
 }
 

diff --git a/src/pkg/net/http/transport_test.go b/src/pkg/net/http/transport_test.go
index cb54a7b419725bc546bbe8c401b6b86bd7e6e55f..21a1f114d33ecf064ba115e6a6bc3a0b9ec106d5 100644 (file)
--- a/src/pkg/net/http/transport_test.go
+++ b/src/pkg/net/http/transport_test.go
@@ -271,6 +271,49 @@ func TestTransportIdleCacheKeys(t *testing.T) {
        }
 }
 
+// Tests that the HTTP transport re-uses connections when a client
+// reads to the end of a response Body without closing it.
+func TestTransportReadToEndReusesConn(t *testing.T) {
+       defer afterTest(t)
+       const msg = "foobar"
+
+       addrSeen := make(map[string]int)
+       ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               addrSeen[r.RemoteAddr]++
+               w.Header().Set("Content-Type", strconv.Itoa(len(msg)))
+               w.WriteHeader(200)
+               w.Write([]byte(msg))
+       }))
+       defer ts.Close()
+
+       buf := make([]byte, len(msg))
+
+       for i := 0; i < 3; i++ {
+               res, err := http.Get(ts.URL)
+               if err != nil {
+                       t.Errorf("Get: %v", err)
+                       continue
+               }
+               // We want to close this body eventually (before the
+               // defer afterTest at top runs), but not before the
+               // len(addrSeen) check at the bottom of this test,
+               // since Closing this early in the loop would risk
+               // making connections be re-used for the wrong reason.
+               defer res.Body.Close()
+
+               if res.ContentLength != int64(len(msg)) {
+                       t.Errorf("res.ContentLength = %d; want %d", res.ContentLength, len(msg))
+               }
+               n, err := res.Body.Read(buf)
+               if n != len(msg) || err != io.EOF {
+                       t.Errorf("Read = %v, %v; want 6, EOF", n, err)
+               }
+       }
+       if len(addrSeen) != 1 {
+               t.Errorf("server saw %d distinct client addresses; want 1", len(addrSeen))
+       }
+}
+
 func TestTransportMaxPerHostIdleConns(t *testing.T) {
        defer afterTest(t)
        resch := make(chan string)