net/http: don't re-use Transport connections if we've seen an EOF

This the second part of making persistent HTTPS connections to
certain servers (notably Amazon) robust.

See the story in part 1: https://golang.org/cl/76400046/

This is the http Transport change that notes whether our
net.Conn.Read has ever seen an EOF. If it has, then we use
that as an additional signal to not re-use that connection (in
addition to the HTTP response headers)

Fixes #3514

LGTM=rsc
R=agl, rsc
CC=golang-codereviews
https://golang.org/cl/79240044

diff --git a/src/pkg/net/http/transport.go b/src/pkg/net/http/transport.go
index bfdd01d0c015a346238e69691eac6564d0631362..3759b88fe0d8c0177c2be0c7b6aba243c6c83e87 100644 (file)
--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -588,7 +588,7 @@ func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) {
                pconn.conn = tlsConn
        }
 
-       pconn.br = bufio.NewReader(pconn.conn)
+       pconn.br = bufio.NewReader(noteEOFReader{pconn.conn, &pconn.sawEOF})
        pconn.bw = bufio.NewWriter(pconn.conn)
        go pconn.readLoop()
        go pconn.writeLoop()
@@ -723,6 +723,7 @@ type persistConn struct {
        tlsState *tls.ConnectionState
        closed   bool                // whether conn has been closed
        br       *bufio.Reader       // from conn
+       sawEOF   bool                // whether we've seen EOF from conn; owned by readLoop
        bw       *bufio.Writer       // to conn
        reqch    chan requestAndChan // written by roundTrip; read by readLoop
        writech  chan writeRequest   // written by roundTrip; read by writeLoop
@@ -841,6 +842,9 @@ func (pc *persistConn) readLoop() {
                                if err != nil {
                                        alive1 = false
                                }
+                               if alive1 && pc.sawEOF {
+                                       alive1 = false
+                               }
                                if alive1 && !pc.t.putIdleConn(pc) {
                                        alive1 = false
                                }
@@ -1134,3 +1138,16 @@ type tlsHandshakeTimeoutError struct{}
 func (tlsHandshakeTimeoutError) Timeout() bool   { return true }
 func (tlsHandshakeTimeoutError) Temporary() bool { return true }
 func (tlsHandshakeTimeoutError) Error() string   { return "net/http: TLS handshake timeout" }
+
+type noteEOFReader struct {
+       r      io.Reader
+       sawEOF *bool
+}
+
+func (nr noteEOFReader) Read(p []byte) (n int, err error) {
+       n, err = nr.r.Read(p)
+       if err == io.EOF {
+               *nr.sawEOF = true
+       }
+       return
+}

diff --git a/src/pkg/net/http/transport_test.go b/src/pkg/net/http/transport_test.go
index 3b8c29a61c65af39acf6af5cb80cb622fbf6300d..a7b461afebb10c2e301a175dc732653e0af16858 100644 (file)
--- a/src/pkg/net/http/transport_test.go
+++ b/src/pkg/net/http/transport_test.go
@@ -11,6 +11,7 @@ import (
        "bytes"
        "compress/gzip"
        "crypto/rand"
+       "crypto/tls"
        "errors"
        "fmt"
        "io"
@@ -1836,6 +1837,71 @@ func TestTransportTLSHandshakeTimeout(t *testing.T) {
        }
 }
 
+// Trying to repro golang.org/issue/3514
+func TestTLSServerClosesConnection(t *testing.T) {
+       defer afterTest(t)
+       closedc := make(chan bool, 1)
+       ts := httptest.NewTLSServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               if strings.Contains(r.URL.Path, "/keep-alive-then-die") {
+                       conn, _, _ := w.(Hijacker).Hijack()
+                       conn.Write([]byte("HTTP/1.1 200 OK\r\nContent-Length: 3\r\n\r\nfoo"))
+                       conn.Close()
+                       closedc <- true
+                       return
+               }
+               fmt.Fprintf(w, "hello")
+       }))
+       defer ts.Close()
+       tr := &Transport{
+               TLSClientConfig: &tls.Config{
+                       InsecureSkipVerify: true,
+               },
+       }
+       defer tr.CloseIdleConnections()
+       client := &Client{Transport: tr}
+
+       var nSuccess = 0
+       var errs []error
+       const trials = 20
+       for i := 0; i < trials; i++ {
+               tr.CloseIdleConnections()
+               res, err := client.Get(ts.URL + "/keep-alive-then-die")
+               if err != nil {
+                       t.Fatal(err)
+               }
+               <-closedc
+               slurp, err := ioutil.ReadAll(res.Body)
+               if err != nil {
+                       t.Fatal(err)
+               }
+               if string(slurp) != "foo" {
+                       t.Errorf("Got %q, want foo", slurp)
+               }
+
+               // Now try again and see if we successfully
+               // pick a new connection.
+               res, err = client.Get(ts.URL + "/")
+               if err != nil {
+                       errs = append(errs, err)
+                       continue
+               }
+               slurp, err = ioutil.ReadAll(res.Body)
+               if err != nil {
+                       errs = append(errs, err)
+                       continue
+               }
+               nSuccess++
+       }
+       if nSuccess > 0 {
+               t.Logf("successes = %d of %d", nSuccess, trials)
+       } else {
+               t.Errorf("All runs failed:")
+       }
+       for _, err := range errs {
+               t.Logf("  err: %v", err)
+       }
+}
+
 func newLocalListener(t *testing.T) net.Listener {
        ln, err := net.Listen("tcp", "127.0.0.1:0")
        if err != nil {