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: document NoBody change in release notes #18257

Closed
jasdel opened this issue Dec 8, 2016 · 23 comments

Comments

Projects
None yet
3 participants
@jasdel
Copy link
Contributor

commented Dec 8, 2016

What version of Go are you using (go version)?

1.8beta1 and devel +2912544

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build416877118=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Make HTTP request with Go 1.8beta1 and Go tip with a zero length HTTP request body.

What did you expect to see?

GET http://route53.amazonaws.com/2013-04-01/hostedzone HTTP/1.1
Host: route53.amazonaws.com
User-Agent: aws-sdk-go/1.6.1 (go1.7.4; darwin; amd64)
Accept-Encoding: gzip

What did you see instead?

GET http://route53.amazonaws.com/2013-04-01/hostedzone HTTP/1.1
Host: route53.amazonaws.com
User-Agent: aws-sdk-go/1.6.1 (go1.8beta1; darwin; amd64)
Transfer-Encoding: chunked
Accept-Encoding: gzip

0

In aws/aws-sdk-go#984 it was discovered that Go 1.8beta1 and tip will send HTTP requests with Transfer-Ecoding: chunked if the request's body is zero length. This seems to be confusing some web servers that are waiting on content from the client. Eventually the the socket is closed by the server.

I think this is related to a HTTP server waiting for the last-chunk value. I have a feeling that the request content encoded body should be sent as, but not familiar with transfer-encoding spec to have a definitive suggesting.

0  <- chunk-size
0  <- last-chunk

https://tools.ietf.org/html/rfc2616#page-25

With that said, it would be preferable for the previous functionality preserved where transfer-encoding is only used for non-zero length bodies.

@jasdel jasdel changed the title HTTP Client using "Transfer-Encoding: chunked" for empty body in Go 1.8, and tip. HTTP Client using "Transfer-Encoding: chunked" for empty GET body in Go 1.8, and tip. Dec 8, 2016

@bradfitz bradfitz changed the title HTTP Client using "Transfer-Encoding: chunked" for empty GET body in Go 1.8, and tip. net/http: Transport sending "Transfer-Encoding: chunked" for empty GET body Dec 8, 2016

@bradfitz bradfitz self-assigned this Dec 8, 2016

@bradfitz bradfitz added this to the Go1.8 milestone Dec 8, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

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

Thanks!

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

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.

@bradfitz bradfitz changed the title net/http: Transport sending "Transfer-Encoding: chunked" for empty GET body net/http: document NoBody change in release notes Dec 8, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

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.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

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

Is that sufficient?

Let me know if I'm missing something.

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

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.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

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?

@gopherbot

This comment has been minimized.

Copy link

commented Dec 8, 2016

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

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

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

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

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

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2016

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.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

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.

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2016

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?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

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.

@gopherbot gopherbot closed this in 9ad47c4 Dec 9, 2016

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2016

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

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?

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2016

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.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

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.

jasdel added a commit to jasdel/aws-sdk-go that referenced this issue Dec 9, 2016

aws/request: Fix for Go 1.8 request incorrectly sent with body
Go 1.8 tightened and clarified the rules code needs to use when building
requests with the http package. Go 1.8 removed the automatic detection
of if the Request.Body was empty, or actually had bytes in it. The SDK
always sets the Request.Body even if it is empty and should not actually
be sent. This is incorrect.

Go 1.8 did add a http.NoBody value that the SDK can use to tell the http
client that the request really should be sent without a body. The
Request.Body cannot be set to nil, which is preferable, because the
field is exported and could introduce nil pointer dereferences for users
of the SDK if they used that field.

Related golang/go#18257

Fix aws#984

jasdel added a commit to aws/aws-sdk-go that referenced this issue Dec 13, 2016

aws/request: Fix for Go 1.8 request incorrectly sent with body (#991)
Go 1.8 tightened and clarified the rules code needs to use when building
requests with the http package. Go 1.8 removed the automatic detection
of if the Request.Body was empty, or actually had bytes in it. The SDK
always sets the Request.Body even if it is empty and should not actually
be sent. This is incorrect.

Go 1.8 added a http.NoBody value that the SDK can use to tell the http
client that the request really should be sent without a body. The
Request.Body cannot be set to nil, which is preferable, because the
field is exported and could introduce nil pointer dereferences for users
of the SDK if they used that field.

This change also deprecates the aws.ReaderSeekerCloser type. This
type has a bug in its design that hides the fact the underlying reader
is not also a seeker. This obfuscation, leads to unexpected bugs. If using
this type for operations such as S3.PutObject it is suggested to use
s3manager.Upload instead. The s3manager.Upload takes an io.Reader
to make streaming to S3 easier.

Related golang/go#18257

Fix #984
@gopherbot

This comment has been minimized.

Copy link

commented Dec 22, 2016

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

gopherbot pushed a commit that referenced this issue Dec 22, 2016

net/http: restore Transport's Request.Body byte sniff in limited cases
In Go 1.8, we'd removed the Transport's Request.Body
one-byte-Read-sniffing to disambiguate between non-nil Request.Body
with a ContentLength of 0 or -1. Previously, we tried to see whether a
ContentLength of 0 meant actually zero, or just an unset by reading a
single byte of the Request.Body and then stitching any read byte back
together with the original Request.Body.

That historically has caused many problems due to either data races,
blocking forever (#17480), or losing bytes (#17071). Thus, we removed
it in both HTTP/1 and HTTP/2 in Go 1.8. Unfortunately, during the Go
1.8 beta, we've found that a few people have gotten bitten by the
behavior change on requests with methods typically not containing
request bodies (e.g. GET, HEAD, DELETE). The most popular example is
the aws-go SDK, which always set http.Request.Body to a non-nil value,
even on such request methods. That was causing Go 1.8 to send such
requests with Transfer-Encoding chunked bodies, with zero bytes,
confusing popular servers (including but limited to AWS).

This CL partially reverts the no-byte-sniffing behavior and restores
it only for GET/HEAD/DELETE/etc requests, and only when there's no
Transfer-Encoding set, and the Content-Length is 0 or -1.

Updates #18257 (aws-go) bug
And also private bug reports about non-AWS issues.

Updates #18407 also, but haven't yet audited things enough to declare
it fixed.

Change-Id: Ie5284d3e067c181839b31faf637eee56e5738a6a
Reviewed-on: https://go-review.googlesource.com/34668
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 22, 2016

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.

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2016

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

@jasdel

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

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.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

@jasdel, great, thanks for the update!

@golang golang locked and limited conversation to collaborators Jan 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.