From: Brad Fitzpatrick Date: Fri, 22 Jul 2016 22:51:05 +0000 (+0000) Subject: net/http: make Transport.RoundTrip return raw Conn.Read error on peek failure X-Git-Tag: go1.7rc4~1^2~12 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=ea2376fcea0be75c856ebd199c0ad0f98192d406;p=gostls13.git 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 Reviewed-by: Ian Lance Taylor Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- diff --git a/src/net/http/transport.go b/src/net/http/transport.go index a51f1d0658..009f3c5b6a 100644 --- a/src/net/http/transport.go +++ b/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 } diff --git a/src/net/http/transport_internal_test.go b/src/net/http/transport_internal_test.go index a157d90630..a05ca6ed0d 100644 --- a/src/net/http/transport_internal_test.go +++ b/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 { diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index 72b98f16d7..749d4530b8 100644 --- a/src/net/http/transport_test.go +++ b/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()