Commit 9296d4ef authored by Brad Fitzpatrick's avatar Brad Fitzpatrick

net/http: don't retry Transport requests if they have a body

This rolls back https://golang.org/cl/27117 partly, softening it so it
only retries POST/PUT/DELETE etc requests where there's no Body (nil
or NoBody). This is a little useless, since most idempotent requests
have a body (except maybe DELETE), but it's late in the Go 1.8 release
cycle and I want to do the proper fix.

The proper fix will look like what we did for http2 and only retrying
the request if Request.GetBody is defined, and then creating a new request
for the next attempt. See https://golang.org/cl/33971 for the http2 fix.

Updates #15723
Fixes #18239
Updates #18241

Change-Id: I6ebaa1fd9b19b5ccb23c8d9e7b3b236e71cf57f3
Reviewed-on: https://go-review.googlesource.com/34134
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: 's avatarTom Bergan <tombergan@google.com>
parent 67b29279
...@@ -1311,7 +1311,8 @@ crypto/x509: return error for missing SerialNumber (CL 27238) ...@@ -1311,7 +1311,8 @@ crypto/x509: return error for missing SerialNumber (CL 27238)
<li><!-- CL 27117 --> <li><!-- CL 27117 -->
The <code>Transport</code> will now retry non-idempotent The <code>Transport</code> will now retry non-idempotent
requests if no bytes were written before a network failure. requests if no bytes were written before a network failure
and the request has no body.
</li> </li>
<li><!-- CL 32481 --> <li><!-- CL 32481 -->
......
...@@ -26,6 +26,7 @@ import ( ...@@ -26,6 +26,7 @@ import (
"strconv" "strconv"
"strings" "strings"
"sync" "sync"
"sync/atomic"
"testing" "testing"
"time" "time"
) )
...@@ -1738,3 +1739,76 @@ func TestClientRedirectTypes(t *testing.T) { ...@@ -1738,3 +1739,76 @@ func TestClientRedirectTypes(t *testing.T) {
res.Body.Close() res.Body.Close()
} }
} }
// issue18239Body is an io.ReadCloser for TestTransportBodyReadError.
// Its Read returns readErr and increments *readCalls atomically.
// Its Close returns nil and increments *closeCalls atomically.
type issue18239Body struct {
readCalls *int32
closeCalls *int32
readErr error
}
func (b issue18239Body) Read([]byte) (int, error) {
atomic.AddInt32(b.readCalls, 1)
return 0, b.readErr
}
func (b issue18239Body) Close() error {
atomic.AddInt32(b.closeCalls, 1)
return nil
}
// Issue 18239: make sure the Transport doesn't retry requests with bodies.
// (Especially if Request.GetBody is not defined.)
func TestTransportBodyReadError(t *testing.T) {
setParallel(t)
defer afterTest(t)
ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
if r.URL.Path == "/ping" {
return
}
buf := make([]byte, 1)
n, err := r.Body.Read(buf)
w.Header().Set("X-Body-Read", fmt.Sprintf("%v, %v", n, err))
}))
defer ts.Close()
tr := &Transport{}
defer tr.CloseIdleConnections()
c := &Client{Transport: tr}
// Do one initial successful request to create an idle TCP connection
// for the subsequent request to reuse. (The Transport only retries
// requests on reused connections.)
res, err := c.Get(ts.URL + "/ping")
if err != nil {
t.Fatal(err)
}
res.Body.Close()
var readCallsAtomic int32
var closeCallsAtomic int32 // atomic
someErr := errors.New("some body read error")
body := issue18239Body{&readCallsAtomic, &closeCallsAtomic, someErr}
req, err := NewRequest("POST", ts.URL, body)
if err != nil {
t.Fatal(err)
}
_, err = tr.RoundTrip(req)
if err != someErr {
t.Errorf("Got error: %v; want Request.Body read error: %v", err, someErr)
}
// And verify that our Body wasn't used multiple times, which
// would indicate retries. (as it buggily was during part of
// Go 1.8's dev cycle)
readCalls := atomic.LoadInt32(&readCallsAtomic)
closeCalls := atomic.LoadInt32(&closeCallsAtomic)
if readCalls != 1 {
t.Errorf("read calls = %d; want 1", readCalls)
}
if closeCalls != 1 {
t.Errorf("close calls = %d; want 1", closeCalls)
}
}
...@@ -1384,7 +1384,7 @@ func (pc *persistConn) closeConnIfStillIdle() { ...@@ -1384,7 +1384,7 @@ func (pc *persistConn) closeConnIfStillIdle() {
// //
// The startBytesWritten value should be the value of pc.nwrite before the roundTrip // The startBytesWritten value should be the value of pc.nwrite before the roundTrip
// started writing the request. // started writing the request.
func (pc *persistConn) mapRoundTripErrorFromReadLoop(startBytesWritten int64, err error) (out error) { func (pc *persistConn) mapRoundTripErrorFromReadLoop(req *Request, startBytesWritten int64, err error) (out error) {
if err == nil { if err == nil {
return nil return nil
} }
...@@ -1399,7 +1399,7 @@ func (pc *persistConn) mapRoundTripErrorFromReadLoop(startBytesWritten int64, er ...@@ -1399,7 +1399,7 @@ func (pc *persistConn) mapRoundTripErrorFromReadLoop(startBytesWritten int64, er
} }
if pc.isBroken() { if pc.isBroken() {
<-pc.writeLoopDone <-pc.writeLoopDone
if pc.nwrite == startBytesWritten { if pc.nwrite == startBytesWritten && req.outgoingLength() == 0 {
return nothingWrittenError{err} return nothingWrittenError{err}
} }
} }
...@@ -1410,7 +1410,7 @@ func (pc *persistConn) mapRoundTripErrorFromReadLoop(startBytesWritten int64, er ...@@ -1410,7 +1410,7 @@ func (pc *persistConn) mapRoundTripErrorFromReadLoop(startBytesWritten int64, er
// up to Transport.RoundTrip method when persistConn.roundTrip sees // up to Transport.RoundTrip method when persistConn.roundTrip sees
// its pc.closech channel close, indicating the persistConn is dead. // its pc.closech channel close, indicating the persistConn is dead.
// (after closech is closed, pc.closed is valid). // (after closech is closed, pc.closed is valid).
func (pc *persistConn) mapRoundTripErrorAfterClosed(startBytesWritten int64) error { func (pc *persistConn) mapRoundTripErrorAfterClosed(req *Request, startBytesWritten int64) error {
if err := pc.canceled(); err != nil { if err := pc.canceled(); err != nil {
return err return err
} }
...@@ -1428,7 +1428,7 @@ func (pc *persistConn) mapRoundTripErrorAfterClosed(startBytesWritten int64) err ...@@ -1428,7 +1428,7 @@ func (pc *persistConn) mapRoundTripErrorAfterClosed(startBytesWritten int64) err
// see if we actually managed to write anything. If not, we // see if we actually managed to write anything. If not, we
// can retry the request. // can retry the request.
<-pc.writeLoopDone <-pc.writeLoopDone
if pc.nwrite == startBytesWritten { if pc.nwrite == startBytesWritten && req.outgoingLength() == 0 {
return nothingWrittenError{err} return nothingWrittenError{err}
} }
...@@ -1710,7 +1710,7 @@ func (pc *persistConn) writeLoop() { ...@@ -1710,7 +1710,7 @@ func (pc *persistConn) writeLoop() {
} }
if err != nil { if err != nil {
wr.req.Request.closeBody() wr.req.Request.closeBody()
if pc.nwrite == startBytesWritten { if pc.nwrite == startBytesWritten && wr.req.outgoingLength() == 0 {
err = nothingWrittenError{err} err = nothingWrittenError{err}
} }
} }
...@@ -1911,14 +1911,14 @@ WaitResponse: ...@@ -1911,14 +1911,14 @@ WaitResponse:
respHeaderTimer = timer.C respHeaderTimer = timer.C
} }
case <-pc.closech: case <-pc.closech:
re = responseAndError{err: pc.mapRoundTripErrorAfterClosed(startBytesWritten)} re = responseAndError{err: pc.mapRoundTripErrorAfterClosed(req.Request, startBytesWritten)}
break WaitResponse break WaitResponse
case <-respHeaderTimer: case <-respHeaderTimer:
pc.close(errTimeout) pc.close(errTimeout)
re = responseAndError{err: errTimeout} re = responseAndError{err: errTimeout}
break WaitResponse break WaitResponse
case re = <-resc: case re = <-resc:
re.err = pc.mapRoundTripErrorFromReadLoop(startBytesWritten, re.err) re.err = pc.mapRoundTripErrorFromReadLoop(req.Request, startBytesWritten, re.err)
break WaitResponse break WaitResponse
case <-cancelChan: case <-cancelChan:
pc.t.CancelRequest(req.Request) pc.t.CancelRequest(req.Request)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment