[go] net/http: rewind request body unconditionally
Gobot Gobot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.
To view, visit change 131755. To unsubscribe, or for help writing mail filters, visit settings.
Aleksandr Razumov (Gerrit)
Aleksandr Razumov has uploaded this change for review.
net/http: rewind request body unconditionally
When http2 fails with ErrNoCachedConn the request is retried with body
that has already been read.
Fixes #25009
Change-Id: I51ed5c8cf469dd8b17c73fff6140ab80162bf267
---
M src/net/http/transport.go
M src/net/http/transport_internal_test.go
2 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index ffe4cdc..c534477 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -477,9 +477,8 @@
}
testHookRoundTripRetried()
- // Rewind the body if we're able to. (HTTP/2 does this itself so we only
- // need to do it for HTTP/1.1 connections.)
- if req.GetBody != nil && pconn.alt == nil {
+ // Rewind the body if we're able to.
+ if req.GetBody != nil {
newReq := *req
var err error
newReq.Body, err = req.GetBody()
diff --git a/src/net/http/transport_internal_test.go b/src/net/http/transport_internal_test.go
index a5f29c9..92729e6 100644
--- a/src/net/http/transport_internal_test.go
+++ b/src/net/http/transport_internal_test.go
@@ -7,8 +7,13 @@
package http
import (
+ "bytes"
+ "crypto/tls"
"errors"
+ "io"
+ "io/ioutil"
"net"
+ "net/http/internal"
"strings"
"testing"
)
@@ -178,3 +183,81 @@
}
}
}
+
+type roundTripFunc func(r *Request) (*Response, error)
+
+func (f roundTripFunc) RoundTrip(r *Request) (*Response, error) {
+ return f(r)
+}
+
+// Issue 25009
+func TestTransportBodyAltRewind(t *testing.T) {
+ cert, err := tls.X509KeyPair(internal.LocalhostCert, internal.LocalhostKey)
+ if err != nil {
+ t.Fatal(err)
+ }
+ ln := newLocalListener(t)
+ defer ln.Close()
+
+ go func() {
+ tln := tls.NewListener(ln, &tls.Config{
+ NextProtos: []string{"foo"},
+ Certificates: []tls.Certificate{cert},
+ })
+ for i := 0; i < 2; i++ {
+ sc, err := tln.Accept()
+ if err != nil {
+ t.Error(err)
+ return
+ }
+ if err := sc.(*tls.Conn).Handshake(); err != nil {
+ t.Error(err)
+ return
+ }
+ sc.Close()
+ }
+ }()
+
+ addr := ln.Addr().String()
+ req, _ := NewRequest("POST", "https://example.org/", bytes.NewBufferString("request"))
+ roundTripped := false
+ tr := &Transport{
+ DisableKeepAlives: true,
+ TLSNextProto: map[string]func(string, *tls.Conn) RoundTripper{
+ "foo": func(authority string, c *tls.Conn) RoundTripper {
+ return roundTripFunc(func(r *Request) (*Response, error) {
+ n, _ := io.Copy(ioutil.Discard, r.Body)
+ if n == 0 {
+ t.Error("body length is zero")
+ }
+ if roundTripped {
+ return &Response{
+ Body: NoBody,
+ StatusCode: 200,
+ }, nil
+ }
+ roundTripped = true
+ return nil, http2noCachedConnError{}
+ })
+ },
+ },
+ DialTLS: func(_, _ string) (net.Conn, error) {
+ tc, err := tls.Dial("tcp", addr, &tls.Config{
+ InsecureSkipVerify: true,
+ NextProtos: []string{"foo"},
+ })
+ if err != nil {
+ return nil, err
+ }
+ if err := tc.Handshake(); err != nil {
+ return nil, err
+ }
+ return tc, nil
+ },
+ }
+ c := &Client{Transport: tr}
+ _, err = c.Do(req)
+ if err != nil {
+ t.Error(err)
+ }
+}
To view, visit change 131755. To unsubscribe, or for help writing mail filters, visit settings.
Iskander Sharipov (Gerrit)
Patch set 1:Run-TryBot +1
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 1:TryBot-Result +1
Aleksandr Razumov (Gerrit)
ping
Brad Fitzpatrick (Gerrit)
Sorry, you mailed this CL at the beginning of my month-long vacation. I'm back next week and will look at it then.
Aleksandr Razumov (Gerrit)
gentle ping (Sorry for doing it again, I just really want that issue to be fixed ^^)
Brad Fitzpatrick (Gerrit)
This is still on my radar. I was out for a month and I'm still catching up. I'm sure this change fixes the bug; I just want to understand the bug first.
Aleksandr Razumov (Gerrit)
Patch Set 1:
This is still on my radar. I was out for a month and I'm still catching up. I'm sure this change fixes the bug; I just want to understand the bug first.
Please also see my comments on github issue: https://github.com/golang/go/issues/25009#issuecomment-415939211
I've tried to describe the bug there in more details. Sorry, forgot to reference it.
Feel free to ask anything regarding that bug here or on github issue, I'm still in context.
Also I'm sure that we can handle that case in more "intelligent" way (e.g. checking for "ErrNoCachedConn" explicitly instead of removing the "pconn.alt == nil" condition entirely), but from my point of view it is more error-prone.
Brad Fitzpatrick (Gerrit)
Patch set 1:Code-Review +2
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick merged this change.
net/http: rewind request body unconditionally
When http2 fails with ErrNoCachedConn the request is retried with body
that has already been read.
Fixes #25009
Change-Id: I51ed5c8cf469dd8b17c73fff6140ab80162bf267
Reviewed-on: https://go-review.googlesource.com/c/131755
Run-TryBot: Iskander Sharipov <iskander...@intel.com>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>
---
M src/net/http/transport.go
M src/net/http/transport_internal_test.go
2 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 7f8fd50..e649303 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -478,9 +478,8 @@