From 8d4d01f0e63d6397c7b3f37f02a864815f29d17f Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 12 Sep 2016 14:44:02 +0000 Subject: [PATCH] [release-branch.go1.7] http2: don't sniff first Request.Body byte in Transport until we have a conn bodyAndLength mutates Request.Body if Request.ContentLength == 0, reading the first byte to determine whether it's actually empty or just undeclared. But we did that before we checked whether our connection was overloaded, which meant the caller could retry the request on an new or lesser-loaded connection, but then lose the first byte of the request. Updates golang/go#17071 (needs bundle into std before fixed) Change-Id: I996a92ad037b45bc49e7cf0801a2027bbbb3c920 Reviewed-on: https://go-review.googlesource.com/29074 Reviewed-by: Gleb Stepanov Reviewed-by: Chris Broadfoot Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-on: https://go-review.googlesource.com/29081 --- http2/transport.go | 6 +++--- http2/transport_test.go | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index ccb885ff0..576203953 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -651,9 +651,6 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) { } hasTrailers := trailers != "" - body, contentLen := bodyAndLength(req) - hasBody := body != nil - cc.mu.Lock() cc.lastActive = time.Now() if cc.closed || !cc.canTakeNewRequestLocked() { @@ -661,6 +658,9 @@ func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) { return nil, errClientConnUnusable } + body, contentLen := bodyAndLength(req) + hasBody := body != nil + // TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere? var requestedGzip bool if !cc.t.disableCompression() && diff --git a/http2/transport_test.go b/http2/transport_test.go index 1bc5c4b4b..bfc54000c 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -2468,3 +2468,24 @@ func TestTransportBodyDoubleEndStream(t *testing.T) { defer res.Body.Close() } } + +// golang.org/issue/17071 -- don't sniff the first byte of the request body +// before we've determined that the ClientConn is usable. +func TestRoundTripDoesntConsumeRequestBodyEarly(t *testing.T) { + const body = "foo" + req, _ := http.NewRequest("POST", "http://foo.com/", ioutil.NopCloser(strings.NewReader(body))) + cc := &ClientConn{ + closed: true, + } + _, err := cc.RoundTrip(req) + if err != errClientConnUnusable { + t.Fatalf("RoundTrip = %v; want errClientConnUnusable", err) + } + slurp, err := ioutil.ReadAll(req.Body) + if err != nil { + t.Errorf("ReadAll = %v", err) + } + if string(slurp) != body { + t.Errorf("Body = %q; want %q", slurp, body) + } +}