]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: make Transport.RoundTrip return raw Conn.Read error on peek failure
authorBrad Fitzpatrick <bradfitz@golang.org>
Fri, 22 Jul 2016 22:51:05 +0000 (22:51 +0000)
committerBrad Fitzpatrick <bradfitz@golang.org>
Tue, 26 Jul 2016 05:28:06 +0000 (05:28 +0000)
From at least Go 1.4 to Go 1.6, Transport.RoundTrip would return the
error value from net.Conn.Read directly when the initial Read (1 byte
Peek) failed while reading the HTTP response, if a request was
outstanding. While never a documented or tested promise, Go 1.7 changed the
behavior (starting at https://golang.org/cl/23160).

This restores the old behavior and adds a test (but no documentation
promises yet) while keeping the fix for spammy logging reported in #15446.

This looks larger than it is: it just changes errServerClosedConn from
a variable to a type, where the type preserves the underlying
net.Conn.Read error, for unwrapping later in Transport.RoundTrip.

Fixes #16465

Change-Id: I6fa018991221e93c0cfe3e4129cb168fbd98bd27
Reviewed-on: https://go-review.googlesource.com/25153
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/net/http/transport.go
src/net/http/transport_internal_test.go
src/net/http/transport_test.go

index a51f1d06580514a0fd991344bd58d55c076776d0..009f3c5b6ab6b6c277f4fc0df2ce908f69b013cc 100644 (file)
@@ -383,6 +383,11 @@ func (t *Transport) RoundTrip(req *Request) (*Response, error) {
                        return resp, nil
                }
                if !pconn.shouldRetryRequest(req, err) {
+                       // Issue 16465: return underlying net.Conn.Read error from peek,
+                       // as we've historically done.
+                       if e, ok := err.(transportReadFromServerError); ok {
+                               err = e.err
+                       }
                        return nil, err
                }
                testHookRoundTripRetried()
@@ -415,11 +420,19 @@ func (pc *persistConn) shouldRetryRequest(req *Request, err error) bool {
                // first, per golang.org/issue/15723
                return false
        }
-       if _, ok := err.(nothingWrittenError); ok {
+       switch err.(type) {
+       case nothingWrittenError:
                // We never wrote anything, so it's safe to retry.
                return true
+       case transportReadFromServerError:
+               // We got some non-EOF net.Conn.Read failure reading
+               // the 1st response byte from the server.
+               return true
        }
-       if err == errServerClosedIdle || err == errServerClosedConn {
+       if err == errServerClosedIdle {
+               // The server replied with io.EOF while we were trying to
+               // read the response. Probably an unfortunately keep-alive
+               // timeout, just as the client was writing a request.
                return true
        }
        return false // conservatively
@@ -566,10 +579,25 @@ var (
        errCloseIdleConns     = errors.New("http: CloseIdleConnections called")
        errReadLoopExiting    = errors.New("http: persistConn.readLoop exiting")
        errServerClosedIdle   = errors.New("http: server closed idle connection")
-       errServerClosedConn   = errors.New("http: server closed connection")
        errIdleConnTimeout    = errors.New("http: idle connection timeout")
 )
 
+// transportReadFromServerError is used by Transport.readLoop when the
+// 1 byte peek read fails and we're actually anticipating a response.
+// Usually this is just due to the inherent keep-alive shut down race,
+// where the server closed the connection at the same time the client
+// wrote. The underlying err field is usually io.EOF or some
+// ECONNRESET sort of thing which varies by platform. But it might be
+// the user's custom net.Conn.Read error too, so we carry it along for
+// them to return from Transport.RoundTrip.
+type transportReadFromServerError struct {
+       err error
+}
+
+func (e transportReadFromServerError) Error() string {
+       return fmt.Sprintf("net/http: Transport failed to read from server: %v", e.err)
+}
+
 func (t *Transport) putOrCloseIdleConn(pconn *persistConn) {
        if err := t.tryPutIdleConn(pconn); err != nil {
                pconn.close(err)
@@ -1293,7 +1321,10 @@ func (pc *persistConn) mapRoundTripErrorFromReadLoop(startBytesWritten int64, er
        if pc.isCanceled() {
                return errRequestCanceled
        }
-       if err == errServerClosedIdle || err == errServerClosedConn {
+       if err == errServerClosedIdle {
+               return err
+       }
+       if _, ok := err.(transportReadFromServerError); ok {
                return err
        }
        if pc.isBroken() {
@@ -1314,7 +1345,11 @@ func (pc *persistConn) mapRoundTripErrorAfterClosed(startBytesWritten int64) err
                return errRequestCanceled
        }
        err := pc.closed
-       if err == errServerClosedIdle || err == errServerClosedConn {
+       if err == errServerClosedIdle {
+               // Don't decorate
+               return err
+       }
+       if _, ok := err.(transportReadFromServerError); ok {
                // Don't decorate
                return err
        }
@@ -1383,7 +1418,7 @@ func (pc *persistConn) readLoop() {
                if err == nil {
                        resp, err = pc.readResponse(rc, trace)
                } else {
-                       err = errServerClosedConn
+                       err = transportReadFromServerError{err}
                        closeErr = err
                }
 
index a157d906300a2cd87d4ca7a0788cd0f92bf35cbd..a05ca6ed0d869adc29133824cd48be0fb87770e4 100644 (file)
@@ -46,17 +46,22 @@ func TestTransportPersistConnReadLoopEOF(t *testing.T) {
        conn.Close() // simulate the server hanging up on the client
 
        _, err = pc.roundTrip(treq)
-       if err != errServerClosedConn && err != errServerClosedIdle {
+       if !isTransportReadFromServerError(err) && err != errServerClosedIdle {
                t.Fatalf("roundTrip = %#v, %v; want errServerClosedConn or errServerClosedIdle", err, err)
        }
 
        <-pc.closech
        err = pc.closed
-       if err != errServerClosedConn && err != errServerClosedIdle {
+       if !isTransportReadFromServerError(err) && err != errServerClosedIdle {
                t.Fatalf("pc.closed = %#v, %v; want errServerClosedConn or errServerClosedIdle", err, err)
        }
 }
 
+func isTransportReadFromServerError(err error) bool {
+       _, ok := err.(transportReadFromServerError)
+       return ok
+}
+
 func newLocalListener(t *testing.T) net.Listener {
        ln, err := net.Listen("tcp", "127.0.0.1:0")
        if err != nil {
index 72b98f16d7eaa0ca9b726034c807e5fd0daa598a..749d4530b84063c41169b8bb7d2d26fcdc9cc1de 100644 (file)
@@ -3511,6 +3511,45 @@ func TestTransportIdleConnTimeout(t *testing.T) {
        }
 }
 
+type funcConn struct {
+       net.Conn
+       read  func([]byte) (int, error)
+       write func([]byte) (int, error)
+}
+
+func (c funcConn) Read(p []byte) (int, error)  { return c.read(p) }
+func (c funcConn) Write(p []byte) (int, error) { return c.write(p) }
+func (c funcConn) Close() error                { return nil }
+
+// Issue 16465: Transport.RoundTrip should return the raw net.Conn.Read error from Peek
+// back to the caller.
+func TestTransportReturnsPeekError(t *testing.T) {
+       errValue := errors.New("specific error value")
+
+       wrote := make(chan struct{})
+       var wroteOnce sync.Once
+
+       tr := &Transport{
+               Dial: func(network, addr string) (net.Conn, error) {
+                       c := funcConn{
+                               read: func([]byte) (int, error) {
+                                       <-wrote
+                                       return 0, errValue
+                               },
+                               write: func(p []byte) (int, error) {
+                                       wroteOnce.Do(func() { close(wrote) })
+                                       return len(p), nil
+                               },
+                       }
+                       return c, nil
+               },
+       }
+       _, err := tr.RoundTrip(httptest.NewRequest("GET", "http://fake.tld/", nil))
+       if err != errValue {
+               t.Errorf("error = %#v; want %v", err, errValue)
+       }
+}
+
 var errFakeRoundTrip = errors.New("fake roundtrip")
 
 type funcRoundTripper func()