net/http: NewRequest started setting ContentLength to -1 and breaking people?

[bradfitz]

Since Go 1 (1.0), there has been this line in transfer.go:

https://github.com/golang/go/blob/release-branch.go1/src/pkg/net/http/transfer.go

		if rr.ContentLength != 0 && rr.Body == nil {
			return nil, fmt.Errorf("http: Request.ContentLength=%d with nil Body", rr.ContentLength)
		}

Which causes a Request.Write of a Request with a nil Body and non-zero length to fail.

But the Transport accepts that, and makes a nil Body imply a ContentLength of 0.

This program should probably have the same output:

package main

import (
	"bytes"
	"log"
	"net/http"
	"net/http/httputil"
)

func main() {
	req, _ := http.NewRequest("GET", "http://foo.com/", nil)
	req.Body = nil
	req.ContentLength = -1

	// Using Request.Write
	var writeBuf bytes.Buffer
	if err := req.Write(&writeBuf); err != nil {
		log.Printf("Request.Write = %v", err)
	} else {
		log.Printf("Request.Write = %s", writeBuf.Bytes())
	}

	// Using Transport (DumpRequestOut uses Transport)
	dump, err := httputil.DumpRequestOut(req, true)
	if err != nil {
		log.Printf("Dump = %v", err)
	} else {
		log.Printf("Dump = %s", dump)
	}
}

But it says:

2016/11/30 18:36:14 Request.Write = http: Request.ContentLength=-1 with nil Body
2016/11/30 18:36:14 Dump = GET / HTTP/1.1
Host: foo.com
User-Agent: Go-http-client/1.1
Transfer-Encoding: chunked
Accept-Encoding: gzip

For Go tip, Go 1.7, Go 1.6, etc.

Decide whether this is okay or we should fix.

[bradfitz]

(This is from investigation motivated by https://golang.org/cl/33577)

[gopherbot]

CL https://golang.org/cl/33714 mentions this issue.

[ggaaooppeenngg]

package main
import (
       	"net/http"
)
func main() {
       	req, err := http.NewRequest("GET", "http://www.example.com", http.NoBody)
       	if err != nil {
       		panic(err)
       	}
       	req.Body = nil
       	res, err := http.DefaultClient.Do(req)
       	if err != nil {
       		panic(err)
       	}
       	defer res.Body.Close()
}

Try this code, this works with normal releases, but break with devel branch.

[ggaaooppeenngg]

BTW, what does draft mean? I already replied this on issue comment, but it seems no one can see it?

[bradfitz]

Draft means "a preliminary version of a piece of writing". Yes, we never saw that because it wasn't mailed. In Gerrit, you can write many comments in different places and then send it all together in one batch.

[bradfitz]

Try this code, this works with normal releases, but break with devel branch.

That can't possibly work in "normal releases", because http.NoBody is new in Go 1.8.

Please give me an example of code that both compiles & works in Go 1.7, but doesn't work at tip ("the devel branch").

[ggaaooppeenngg]

package main

import (
       	"io"
       	"net/http"
)

var AnyUserImplementReader = anyUserImplementReader{}

type anyUserImplementReader struct{}

func (anyUserImplementReader) Read([]byte) (int, error)         { return 0, io.EOF }
func (anyUserImplementReader) Close() error                     { return nil }
func (anyUserImplementReader) WriteTo(io.Writer) (int64, error) { return 0, nil }

func main() {
       	req, err := http.NewRequest("GET", "http://www.example.com", AnyUserImplementReader)
       	if err != nil {
       		panic(err)
       	}
       	req.Body = nil
       	res, err := http.DefaultClient.Do(req)
       	if err != nil {
       		panic(err)
       	}
       	defer res.Body.Close()
}

Any user implemented reader that is not in the list of *bytes.Buffer,*bytes.Reader,*strings.Reader will cause this problem.

The difference is that you set ContentLength to -1 if the length can not be told in http.NewRequest, pre-filled ContentLength is added in Go tip, and in the Go1.7 it is 0, no matter what body I set then, it always work.

And this arises my another question, why there should be a pre-filled ContentLength?

[bradfitz]

Ah, great! Now I can reproduce the regression.

I was being misled by the trickery in DumpRequestOut:

func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
        save := req.Body
        dummyBody := false
        if !body || req.Body == nil {
                req.Body = nil
                if req.ContentLength != 0 {
                        req.Body = ioutil.NopCloser(io.LimitReader(neverEnding('x'), req.ContentLength))

My test in the first comment is kinda bogus.

[bradfitz]

@ggaaooppeenngg, I see what changed.

The Transport and Request.Write are still strict. They have been since Go 1.

What's different is that http.NewRequest now sets the ContentLength explicitly to -1 to mean unknown when it's unknown, rather than using 0. I suppose I could remove that.

But does this actually matter? This isn't an API behavior we ever meant to guarantee, that you could use NewRequest and then nil out the field you just set. When does that actually occur?

You never filed a bug (you just sent a change), so I don't know the motivation here.

[gopherbot]

CL https://golang.org/cl/33801 mentions this issue.

[ggaaooppeenngg]

@bradfitz Actually I put a nil body as argument in the io.Reader (user implemented) constructor, then I reset the request body to nil depending on the initial body.

body := nil (a nil body)
req :=  http.NewRequest("GET","http://www.baidu.com",NewReader(body))
if body == nil{
   req.Body = nil
}

Since a interface containing nil is not nil, ContentLength becomes -1 in the Go tip. Actually this can be fixed in this way.

body := nil (a nil body)
if body == nil{
   reader = nil
}else{
   reader = NewReader(body)
}
req :=  http.NewRequest("GET","http://www.baidu.com",reader)
if body == nil{
   req.Body = nil
}

I make the change, because I think it is a good to not break the user space code no matter what is patched, but you can announce this change, so users can check it before update go version.

[bradfitz]

I see. Okay, I'm tempted to revert my revert then (so the codebase can make forward progress) and let users fix their code like you suggested.

Users need to learn about https://golang.org/doc/faq#nil_error eventually.

But I'm also not sure how much it really matters.

[tombergan]

What's different is that http.NewRequest now sets the ContentLength explicitly to -1 to mean unknown when it's unknown, rather than using 0. I suppose I could remove that.

The http.Request doc is unfortunately ambiguous:

// ContentLength records the length of the associated content.
// The value -1 indicates that the length is unknown.
// Values >= 0 indicate that the given number of bytes may
// be read from Body.
// For client requests, a value of 0 means unknown if Body is not nil.
ContentLength  int64

It is confusing ContentLength == -1 means unknown, except for client requests where Body != nil, for which ContentLength == 0 also means unknown. I would expect that ContentLength == -1 means unknown, full stop, while ContentLength == 0 means "empty", full stop.

But does this actually matter? This isn't an API behavior we ever meant to guarantee, that you could use NewRequest and then nil out the field you just set. When does that actually occur?

I found old code that does exactly this :-/ The code breaks when testing against 1.8 before 4bc7b5a. The code is roughly:

func CreateProxyRequest(req *http.Request) *http.Request {
  req.Body = unknownWrapper(req.Body)  // in caller
  if req.Method == "POST" {
    outreq = http.NewRequest(req.Method, req.URL.String(), req.Body)
  } else {
    outreq = http.NewRequest(rew.Method, req.URL.String(), nil /* disallow bodies otherwise */)
  }
  ...
  if outreq.Body != nil && outreq.ContentLength == 0 /* here's the problem */ {
    outreq.ContentLength = parseCL(outreq.Header)
    // instead of doing this, the code should set outreq.ContentLength = req.ContentLength
    // in the "POST" branch, above
  }
  return outreq
}

I submit this code as further evidence that the API may have ossified to the point that 4bc7b5a should not be reverted. That is, reverting 4bc7b5a will likely break actually code.

That said, I am in favor of reverting 4bc7b5a. We will break code, but I can't imagine that we will break code that isn't already broken in other ways, and reverting 4bc7b5a is much cleaner in the long run.