net/http: make Transport.RoundTrip return raw Conn.Read error on peek…

… failure

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 <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

src/net/http/transport.go

@@ -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
 		}
 

src/net/http/transport_internal_test.go

@@ -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 {

src/net/http/transport_test.go

@@ -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()