code review 79240044: net/http: don't re-use Transport connections if we've s...

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://codereview.appspot.com/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 Issue 3514

[bradfitz, 2014-03-24 02:46:08]

Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2014-03-25 15:35:39]

Can I get a review on this one too?

Copying Russ, who I talked to last night about these changes, and David,
who's reviewed a fair bit of http.



On Sun, Mar 23, 2014 at 7:46 PM, <bradfitz@golang.org> wrote:

> Reviewers: agl,
>
> Message:
> Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> 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://codereview.appspot.com/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 Issue 3514
>
> Please review this at https://codereview.appspot.com/79240044/
>
> Affected files (+84, -1 lines):
>   M src/pkg/net/http/transport.go
>   M src/pkg/net/http/transport_test.go
>
>
> Index: src/pkg/net/http/transport.go
> ===================================================================
> --- a/src/pkg/net/http/transport.go
> +++ b/src/pkg/net/http/transport.go
> @@ -588,7 +588,7 @@
>                 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 @@
>         tlsState *tls.ConnectionState
>         closed   bool                // whether conn has been closed
>         br       *bufio.Reader       // from conn
> +       sawEOF   bool                // whether we've seen EOF from conn
>         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 @@
>                                 if err != nil {
>                                         alive1 = false
>                                 }
> +                               if alive1 && pc.sawEOF {
> +                                       alive1 = false
> +                               }
>                                 if alive1 && !pc.t.putIdleConn(pc) {
>                                         alive1 = false
>                                 }
> @@ -1134,3 +1138,16 @@
>  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
> +}
> Index: src/pkg/net/http/transport_test.go
> ===================================================================
> --- a/src/pkg/net/http/transport_test.go
> +++ b/src/pkg/net/http/transport_test.go
> @@ -11,6 +11,7 @@
>         "bytes"
>         "compress/gzip"
>         "crypto/rand"
> +       "crypto/tls"
>         "errors"
>         "fmt"
>         "io"
> @@ -1836,6 +1837,71 @@
>         }
>  }
>
> +// 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 {
>
>
>

[rsc, 2014-03-25 15:39:28]

LGTM

[rsc, 2014-03-25 15:43:40]

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:770: pb, err := pc.br.Peek(1)
Is there a missing check for EOF (or really any error) here?
Perhaps that is the better fix?

[bradfitz, 2014-03-25 16:14:04]

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:770: pb, err := pc.br.Peek(1)
On 2014/03/25 15:43:40, rsc wrote:
> Is there a missing check for EOF (or really any error) here?
> Perhaps that is the better fix?

It's there, but far away. See lines 787 and 799.

[agl1, 2014-03-25 16:14:15]

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:726: sawEOF   bool                // whether we've
seen EOF from conn
How is this synchronised? If by lk, then should it be below it and the
"following 3 fields" comment be updated?

[bradfitz, 2014-03-25 16:17:55]

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:726: sawEOF   bool                // whether we've
seen EOF from conn
On 2014/03/25 16:14:16, agl1 wrote:
> How is this synchronised? If by lk, then should it be below it and the
> "following 3 fields" comment be updated?

It's owned (read + written) by the single reading goroutine.  That goroutine is
either in a Read (and thus maybe writing this field, sandwiched between other
Reads) or reading this field, before it goes back into another Read.

[rsc, 2014-03-25 16:18:50]

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:770: pb, err := pc.br.Peek(1)
On 2014/03/25 16:14:04, bradfitz wrote:
> On 2014/03/25 15:43:40, rsc wrote:
> > Is there a missing check for EOF (or really any error) here?
> > Perhaps that is the better fix?
> 
> It's there, but far away. See lines 787 and 799.

That won't happen until after the <-pc.reqch, which means something else has
already tried to use this connection, even though we may know at this line that
the connection is dead. That <- may be a long delay. It's not just far away in
code but also in time.

Let's resolve this question before submitting either of the two CLs.

[bradfitz, 2014-03-25 16:27:56]

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:770: pb, err := pc.br.Peek(1)
On 2014/03/25 16:18:50, rsc wrote:
> On 2014/03/25 16:14:04, bradfitz wrote:
> > On 2014/03/25 15:43:40, rsc wrote:
> > > Is there a missing check for EOF (or really any error) here?
> > > Perhaps that is the better fix?
> > 
> > It's there, but far away. See lines 787 and 799.
> 
> That won't happen until after the <-pc.reqch, which means something else has
> already tried to use this connection, even though we may know at this line
that
> the connection is dead. That <- may be a long delay. It's not just far away in
> code but also in time.
> 
> Let's resolve this question before submitting either of the two CLs.

The block from 772-782 addresses that case (of an EOF before the connection has
been selected).

Then numExpectedResponses == 0 (we haven't selected it yet / wrote on it) and
either it's hanging up on us nicely, or rudely, or it's speaking to us out of
turn (the len(pb) check).  This code used to be more verbose with its logging,
but it was found to be noisy and useless.  We only log for the
speaking-out-of-turn case, which suggests something is really messed up (client
code, or handler code, or their server).

[rsc, 2014-03-25 16:55:11]

LGTM

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/79240044/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:770: pb, err := pc.br.Peek(1)
Great. Okay, then everything I wrote earlier is still true. Wonderful.

[bradfitz, 2014-03-25 17:59:11]

*** Submitted as https://code.google.com/p/go/source/detail?r=af4910816fca ***

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://codereview.appspot.com/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 Issue 3514

LGTM=rsc
R=agl, rsc
CC=golang-codereviews
https://codereview.appspot.com/79240044

[gobot, 2014-03-25 18:34:04]

This CL appears to have broken the windows-386-ec2 builder.
See http://build.golang.org/log/24dd72e022e4b594c6331a658b565cfc25be1ff3

[bradfitz, 2014-03-25 18:51:52]

Real. I tried to make the test tolerant, but I guess I failed. In practice,
on Linux and Mac, with varying GOMAXPROCS, it was getting 17-19 of 20
attempts succeeding, so I set the tolerance to 1 pass per 20. (and before
the patches, it was always 0)

And here Windows shows 0.

I'll look into it more on a Windows machine.





On Tue, Mar 25, 2014 at 11:34 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the windows-386-ec2 builder.
> See http://build.golang.org/log/24dd72e022e4b594c6331a658b565cfc25be1ff3
>
> https://codereview.appspot.com/79240044/
>