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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/net/http/request.go b/src/net/http/request.go
index 2d65ca3c8a19595a142366805076f6211deab603..81763007c4bac200f1a6e2a5ae85531a007e14ae 100644 (file)
--- 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 483c025fb09bd10778659634d99e7916c37ff593..e6748375b587636434d7083fd0797be81980f879 100644 (file)
--- 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 d13e37aba08f7c07b3f93cfc72e90935126ada3c..c398e645392e218c51e0fa92309c03966c3394ad 100644 (file)
--- 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")
        }