http2: use GetBody unconditionally on Transport retry, when available
We were previously only using the new-ish Request.GetBody to "rewind"
a Request.Body on retry when it seemed that we hadn't started the
read+write body copy process from the old request yet.
Apparently there's a bug somewhere, so this is a safe minimal fix for
now, unconditionally using GetBody when it's available, rather than
only using it when it seems we need to. Should have no performance impact
because it's supposed to be cheap, and this only happens on rare retries
where the server's GOAWAY came in-flight while we were writing a request.
Updates golang/go#25009 (not a fix, but enough for Go 1.11)
Change-Id: Ia462944d4a68cf2fde8d32b7b357b450c509a349
Reviewed-on: https://go-review.googlesource.com/123476
Reviewed-by: Ian Lance Taylor <iant@golang.org>
diff --git a/http2/transport.go b/http2/transport.go
index 300b02f..671c187 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -424,27 +424,36 @@
if !canRetryError(err) {
return nil, err
}
- if !afterBodyWrite {
- return req, nil
- }
// If the Body is nil (or http.NoBody), it's safe to reuse
// this request and its Body.
if req.Body == nil || reqBodyIsNoBody(req.Body) {
return req, nil
}
- // Otherwise we depend on the Request having its GetBody
- // func defined.
+
+ // If the request body can be reset back to its original
+ // state via the optional req.GetBody, do that.
getBody := reqGetBody(req) // Go 1.8: getBody = req.GetBody
- if getBody == nil {
- return nil, fmt.Errorf("http2: Transport: cannot retry err [%v] after Request.Body was written; define Request.GetBody to avoid this error", err)
+ if getBody != nil {
+ // TODO: consider a req.Body.Close here? or audit that all caller paths do?
+ body, err := getBody()
+ if err != nil {
+ return nil, err
+ }
+ newReq := *req
+ newReq.Body = body
+ return &newReq, nil
}
- body, err := getBody()
- if err != nil {
- return nil, err
+
+ // The Request.Body can't reset back to the beginning, but we
+ // don't seem to have started to read from it yet, so reuse
+ // the request directly. The "afterBodyWrite" means the
+ // bodyWrite process has started, which becomes true before
+ // the first Read.
+ if !afterBodyWrite {
+ return req, nil
}
- newReq := *req
- newReq.Body = body
- return &newReq, nil
+
+ return nil, fmt.Errorf("http2: Transport: cannot retry err [%v] after Request.Body was written; define Request.GetBody to avoid this error", err)
}
func canRetryError(err error) bool {
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 963a723..afd7969 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -4046,3 +4046,46 @@
func TestClientConnShutdownCancel(t *testing.T) {
testClientConnClose(t, shutdownCancel)
}
+
+// Issue 25009: use Request.GetBody if present, even if it seems like
+// we might not need it. Apparently something else can still read from
+// the original request body. Data race? In any case, rewinding
+// unconditionally on retry is a nicer model anyway and should
+// simplify code in the future (after the Go 1.11 freeze)
+func TestTransportUsesGetBodyWhenPresent(t *testing.T) {
+ calls := 0
+ someBody := func() io.ReadCloser {
+ return struct{ io.ReadCloser }{ioutil.NopCloser(bytes.NewReader(nil))}
+ }
+ req := &http.Request{
+ Body: someBody(),
+ GetBody: func() (io.ReadCloser, error) {
+ calls++
+ return someBody(), nil
+ },
+ }
+
+ afterBodyWrite := false // pretend we haven't read+written the body yet
+ req2, err := shouldRetryRequest(req, errClientConnUnusable, afterBodyWrite)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if calls != 1 {
+ t.Errorf("Calls = %d; want 1", calls)
+ }
+ if req2 == req {
+ t.Error("req2 changed")
+ }
+ if req2 == nil {
+ t.Fatal("req2 is nil")
+ }
+ if req2.Body == nil {
+ t.Fatal("req2.Body is nil")
+ }
+ if req2.GetBody == nil {
+ t.Fatal("req2.GetBody is nil")
+ }
+ if req2.Body == req.Body {
+ t.Error("req2.Body unchanged")
+ }
+}
