Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bradfitz opened this issue Nov 30, 2016 · 13 comments
Closed
Milestone

Comments

@bradfitz
Copy link
Contributor

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 bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 30, 2016
@bradfitz bradfitz added this to the Go1.9 milestone Nov 30, 2016
@bradfitz bradfitz self-assigned this Nov 30, 2016
@bradfitz
Copy link
Contributor Author

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

@gopherbot
Copy link

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

@ggaaooppeenngg
Copy link
Contributor

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
Copy link
Contributor

image

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

@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 1, 2016

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
Copy link
Contributor Author

bradfitz commented Dec 1, 2016

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
Copy link
Contributor

ggaaooppeenngg commented Dec 1, 2016

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 bradfitz modified the milestones: Go1.8, Go1.9 Dec 1, 2016
@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 1, 2016

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.

gopherbot pushed a commit that referenced this issue Dec 1, 2016
NoBody is new in Go 1.8.

Found while investigating #18117

Change-Id: I6bda030f358e2270f090d108cb3a89c8a2665fcb
Reviewed-on: https://go-review.googlesource.com/33714
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 1, 2016

@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.

@bradfitz bradfitz changed the title net/http: Request.Write and Transport differ in Content-Length strictness net/http: NewRequest started setting ContentLength to -1 and breaking people? Dec 1, 2016
@bradfitz bradfitz removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 1, 2016
@gopherbot
Copy link

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

@ggaaooppeenngg
Copy link
Contributor

ggaaooppeenngg commented Dec 2, 2016

@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
Copy link
Contributor Author

bradfitz commented Dec 2, 2016

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
Copy link
Contributor

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.

nstratos added a commit to nstratos/go-myanimelist that referenced this issue Mar 3, 2017
The integration tests revealed that AnimeService.List and
MangaService.List started receiving a 411 Length required error from the
MyAnimeList API. Both methods use mal.Client.NewRequest to create API
requests.

This is probably related to golang/go#18117
and it happened because when using http.NewRequest (inside
mal.Client.NewRequest) with GET, the body argument was not nil as it
should. This commit ensures that nil body is properly passed to
http.NewRequest when mal.Client.NewRequest has no data to encode.
@golang golang locked and limited conversation to collaborators Dec 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants