Whoa, good find! I'm surprised that the nothing in the gauntlet of net/http tests caught that.

Thanks!

Oh, sorry, I'm blind. I missed the most important lines of your repro and I got fixated on worrying about something unrelated.

In your repro, you write:

	req, err := http.NewRequest("GET", server.URL, nil)

	body := bytes.NewReader([]byte{})
	req.Body = ioutil.NopCloser(body)

The first line, NewRequest ends up setting req.Body to nil, and req.ContentLength to 0. So far so good.

Then you set Body non-nil. The only part of the http package with magic that uses the type of the Body is NewRequest. At this point on, the http Transport (as used by the Client) just sees that your Request.ContentLength is 0 (which means nothing for client requests) and sees a non-nil Body, so it assumes it might have content.

In Go 1.7 and some earlier releases, but not all, the Transport first did a test-read of 1 byte to determine whether the ContentLength was actually 0, or was only 0 because it was never set by the client.

That auto-byte-sniffing behavior has caused so many problems, we finally removed it in Go 1.8.

It now does exactly what you tell it to.

A new https://beta.golang.org/pkg/net/http/#NoBody variable can be used to signal that your ContentLength of 0 is actually zero, and not just unknown. It's now used by NewRequest internally. So if you did instead:

	body := bytes.NewReader([]byte{})
	req, err := http.NewRequest("GET", server.URL, body)

... then it would work as you expect, since NewRequest would check the size of the body and set NoBody automatically.

I never added this to https://beta.golang.org/doc/go1.8#net_http because I thought it was too much of an internal detail & more of a bug fix. Apparently not. I'll update the docs.

I forgot to mention: there is also #13722 open, to support GET-with-body more officially in Go 1.9. Consider this a baby step in that directory. Do you depend on GET with body in general? If not, the safest option for you to support any Go version is to never set Body to non-nil for GET requests.

I've sent https://go-review.googlesource.com/34210 as a documentation update.

Is that sufficient?

Let me know if I'm missing something.

Thanks for the update! This use case the code is setting the body arbitrarily for all requests as they are built. In all GET et co, request no body bytes are expected to be sent. This is why the examples had zero length bodies.

Arguably the code shouldn't be setting the request's Body at all if the length of the bytes is zero, which is a change I should make.

My biggest concern was if users update Go to 1.8 the AWS SDK for Go would start to fail to make requests because an empty body was set on Request.Body, which worked fine in Go 1.7.

Maybe I can make an exception for GET requests.

That might be the good opt-in answer to #13722 anyway: for GET requests, require that ContentLength be non-zero for a body to be sent.

Would that work?

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

Do you think that should apply to all non PUT/POST? I think the same issue would occur on DELETE and HEAD as well.

Though I think that might get into dangerous braking change territory. Since code may be relying on GET/HEAD/DELETE requests to automatically figure out that the request's body needs to be sent by inspecting if there are any bytes in the body. Incorrectly or correctly while also ignoring the need to set Request.ContentLength.

/edit: clarity

The current docs for net/http's Request.Body does state a non-nil body implies that the request has a body, but the functionality up till 1.8 has been to inspect the body to determine if it is actually empty like you mentioned. Sending the content as chunked transfer-encoding for GET/HEAD/DELETE if ContentLength is not also set.

Is removing this functionality a breaking change? The doc CL is definitely a great clarification for this situation, but I'm concerned about the impact of the change on existing code that was maybe incorrectly, relying on this functionality.

I think the same issue would occur on DELETE and HEAD as well.

A HEAD request can't have a body. A DELETE can, just like POST or PUT or just about anything besides HEAD. Even though a GET can in theory have a body, conventionally it doesn't, which is why I propose making that method require a more explicit opt-in mechanism (setting the Body non-nil)

Sending the content as chunked transfer-encoding for GET/HEAD/DELETE if ContentLength is not also set.

I don't follow that sentence. But yes, when a ContentLength is unknown (-1 or 0 with non-nil body), then we use chunked encoding, or equivalent, depending on the protocol version.

Is removing this functionality a breaking change? The doc CL is definitely a great clarification for this situation, but I'm concerned about the impact of the change on existing code that was maybe incorrectly, relying on this functionality.

Some breakage for undefined or edge case behavior is sometimes acceptable if it fixes more important bugs and clarifies the documentation in the process. This might be such a case.

I hadn't heard of any breakages until your case.

You could update your code to stop setting your GET bodies non-nil, right? Your users would just need to "go get -u"? Maybe that's not ideal, but it seems within the realm of acceptable, in the name of forward progress.

Maybe there's a hacky workaround I could do just for your case, of using a non-nil Request.Body set to a standard-library-defined type(s). Is your empty non-nil body always a ioutil.NopCloser of *bytes.Reader? I could perhaps just detect that and also treat it as zero deep in the Transport code.

Some breakage for undefined or edge case behavior is sometimes acceptable if it fixes more important bugs and clarifies the documentation in the process. This might be such a case.

Yeah, I think this is what i might need to do. My code is using the fact that in 1.7 a request can be sent without a body even though Request.Body is not nil, and ContentLength is 0. A workaround for 1.8 would be to set the request body to http.NoBody. We cannot set it to nil regrettably because users of the library may try to access that field.

Ideally it would be best not to have a hack in the stdlib :( . My code is using a custom internal reader for the Request Body value, but it does satisfy the io.ReadSeeker interface though. Maybe checking if Request.Body is an io.ReadSeeker and calculating the length from that is an option?

My code is using a custom internal reader for the Request Body value, but it does satisfy the io.ReadSeeker interface though. Maybe checking if Request.Body is an io.ReadSeeker and calculating the length from that is an option?

We can't call any method on something that might block or panic. If it's an in-memory type we know of, we can infer its length, but we've tried to keep that magic to NewRequest only, and we've never expanded its magic set of known types in the past.

Thanks, yeah that makes sense. I'll update my code to use http.NoBody if the body shouldn't be sent.

I will still consider whether to do something about GET (and I suppose DELETE) requests with ContentLength 0 for Go 1.8, though.,

Are there other HTTP methods you use and don't expect to ever send a body?

Thanks, I really think the best way forward for me, if the request life-cycle is being tighten to no longer inspect the length of the body, is to update my code to use http.NoBody for 1.8+. It will require users to update their repo when updating to Go 1.8, but not sure how the stdlib could be updated for each method without introducing additional inconsistencies, or edge/special cases for GET et co.

In my use case the only methods the server cannot handle with chunked bodies are GET, HEAD, and DELETE. In of its self this is probably something that should be fixed server side to not fail chunked requests when the body should be ignored.

Playing around with 1.8 request with the example https://play.golang.org/p/1IllAJN-_j using different methods, and request body empty. In all cases it looks like the requests are sent with the header Transfer-Encoding: chunked, regardless of method. Which makes sense given no ContentLength was set on the request, and length of body is not being inspected. This is similar to 1.7 when ContentLength was not set, and body was not empty, except in 1.8 it also applies to empty body's as well. Including HEAD.

It should not apply to HEAD. A HEAD request can't have a body, ever. If you can come up with an example of a HEAD request attempting to send a body (or even a Transfer-Encoding: chunked header), please file a separate bug (and reference this one).

Oh, sorry, a HEAD can't have a response body, not request. No clue about request. I was just busy playing in the snow and not thinking straight.

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

I've submitted a fix (6e36811) to Go 1.8 (after beta2) for this, so even users who don't update their aws-sdk-go won't be affected.

Thanks a lot for making this update! I'll test against the aws-sdk-go repo.

Hi @bradfitz Thanks for making this change. Sorry for delay getting back to you I've been out for a few weeks. From testing this looks like it solved most of the issue with aws-sdk-go and service requests. In testing we did discovered in aws/aws-sdk-go#1058 that one of the back end services does not correctly handle PUT with chunked transfer encoding, but I've reached out to them investigating if they can correct that issue. I'll still suggest users update to the latest version of aws-sdk-go, but this change should mitigate most of the impact.

@jasdel, great, thanks for the update!