From 4bc7b5aeba2061c14199354b1c59592aa481bac8 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 1 Dec 2016 22:01:35 +0000 Subject: [PATCH] net/http: revert change making NewRequest set ContentLength -1 The introduction of NoBody and related body-peeking bug fixes also added a "cleanup" of sorts to make NewRequest set the returned Requests's ContentLength to -1 when it didn't know it. Using -1 to mean unknown is what the documentation says, but then people apparently(?) depended on it being zero so they could do this: req, _ := http.NewRequest("POST", url, someNonNilReaderWithUnkownSize) req.Body = nil res, err := http.DefaultClient.Do(req) ... and expect it to work. After https://golang.org/cl/31445 the contrived(?) code above stopped working, since Body was nil and ContentLength was -1, which has been disallowed since Go 1.0. So this restores the old behavior of NewRequest, not setting it to -1. That part of the fix isn't required as of https://golang.org/cl/31726 (which added NoBody) I still don't know whether this bug is hypothetical or actually affected people in practice. Let's assume it's real for now. Fixes #18117 Change-Id: I42400856ee92a1a4999b5b4668bef97d885fbb53 Reviewed-on: https://go-review.googlesource.com/33801 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/net/http/request.go | 10 ++++++---- src/net/http/request_test.go | 25 ++++++++----------------- src/net/http/requestwrite_test.go | 10 ++++++---- 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/net/http/request.go b/src/net/http/request.go index 2d65ca3c8a1959..81763007c4bac2 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -785,9 +785,11 @@ func NewRequest(method, urlStr string, body io.Reader) (*Request, error) { return ioutil.NopCloser(&r), nil } default: - if body != NoBody { - req.ContentLength = -1 // unknown - } + // This is where we'd set it to -1 (at least + // if body != NoBody) to mean unknown, but + // that broke people during the Go 1.8 testing + // period. People depend on it being 0 I + // guess. Maybe retry later. See Issue 18117. } // For client requests, Request.ContentLength of 0 // means either actually 0, or unknown. The only way @@ -797,7 +799,7 @@ func NewRequest(method, urlStr string, body io.Reader) (*Request, error) { // so we use a well-known ReadCloser variable instead // and have the http package also treat that sentinel // variable to mean explicitly zero. - if req.ContentLength == 0 { + if req.GetBody != nil && req.ContentLength == 0 { req.Body = NoBody req.GetBody = func() (io.ReadCloser, error) { return NoBody, nil } } diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go index 483c025fb09bd1..e6748375b58763 100644 --- a/src/net/http/request_test.go +++ b/src/net/http/request_test.go @@ -498,10 +498,14 @@ func TestNewRequestContentLength(t *testing.T) { {bytes.NewBuffer([]byte("1234")), 4}, {strings.NewReader("12345"), 5}, {strings.NewReader(""), 0}, - // Not detected: - {struct{ io.Reader }{strings.NewReader("xyz")}, -1}, - {io.NewSectionReader(strings.NewReader("x"), 0, 6), -1}, - {readByte(io.NewSectionReader(strings.NewReader("xy"), 0, 6)), -1}, + {NoBody, 0}, + + // Not detected. During Go 1.8 we tried to make these set to -1, but + // due to Issue 18117, we keep these returning 0, even though they're + // unknown. + {struct{ io.Reader }{strings.NewReader("xyz")}, 0}, + {io.NewSectionReader(strings.NewReader("x"), 0, 6), 0}, + {readByte(io.NewSectionReader(strings.NewReader("xy"), 0, 6)), 0}, } for i, tt := range tests { req, err := NewRequest("POST", "http://localhost/", tt.r) @@ -511,9 +515,6 @@ func TestNewRequestContentLength(t *testing.T) { if req.ContentLength != tt.want { t.Errorf("test[%d]: ContentLength(%T) = %d; want %d", i, tt.r, req.ContentLength, tt.want) } - if (req.ContentLength == 0) != (req.Body == NoBody) { - t.Errorf("test[%d]: ContentLength = %d but Body non-nil is %v", i, req.ContentLength, req.Body != nil) - } } } @@ -825,16 +826,6 @@ func TestNewRequestGetBody(t *testing.T) { } } -func TestNewRequestNoBody(t *testing.T) { - req, err := NewRequest("GET", "http://foo.com/", NoBody) - if err != nil { - t.Fatal(err) - } - if req.ContentLength != 0 { - t.Errorf("ContentLength = %d; want 0", req.ContentLength) - } -} - func testMissingFile(t *testing.T, req *Request) { f, fh, err := req.FormFile("missing") if f != nil { diff --git a/src/net/http/requestwrite_test.go b/src/net/http/requestwrite_test.go index d13e37aba08f7c..c398e645392e21 100644 --- a/src/net/http/requestwrite_test.go +++ b/src/net/http/requestwrite_test.go @@ -581,12 +581,14 @@ func (rc *closeChecker) Close() error { // inside a NopCloser, and that it serializes it correctly. func TestRequestWriteClosesBody(t *testing.T) { rc := &closeChecker{Reader: strings.NewReader("my body")} - req, _ := NewRequest("POST", "http://foo.com/", rc) - if req.ContentLength != -1 { - t.Errorf("got req.ContentLength %d, want -1", req.ContentLength) + req, err := NewRequest("POST", "http://foo.com/", rc) + if err != nil { + t.Fatal(err) } buf := new(bytes.Buffer) - req.Write(buf) + if err := req.Write(buf); err != nil { + t.Error(err) + } if !rc.closed { t.Error("body not closed after write") }