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: Request.ProtoMajor/Minor behavior vs docs discrepancy #18407

Closed
bradfitz opened this issue Dec 21, 2016 · 2 comments
Closed

net/http: Request.ProtoMajor/Minor behavior vs docs discrepancy #18407

bradfitz opened this issue Dec 21, 2016 · 2 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

The net/http.Request docs say:

        // The protocol version for incoming server requests.
        //
        // For client requests these fields are ignored. The HTTP
        // client code always uses either HTTP/1.1 or HTTP/2.
        // See the docs on Transport for details.
        Proto      string // "HTTP/1.0"
        ProtoMajor int    // 1
        ProtoMinor int    // 0

But that turns out not to be true. transfer.go's func newTransferWriter checks for instance:

        case *Request:
...
                atLeastHTTP11 = rr.ProtoAtLeast(1, 1)
...
                if t.ContentLength < 0 && len(t.TransferEncoding) == 0 && atLeastHTTP11 { 
                        t.TransferEncoding = []string{"chunked"} 
                }

This bug is to audit investigate either fixing the implementation (if possible), or fixing the docs, and auditing any other locations. Hopefully it's fixable and doesn't require documentation.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 21, 2016
@bradfitz bradfitz added this to the Go1.9 milestone Dec 21, 2016
@bradfitz bradfitz self-assigned this Dec 21, 2016
@gopherbot
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor changed the title net/http: Request.ProtoMajor/Minor behavior vs docs discrepenancy net/http: Request.ProtoMajor/Minor behavior vs docs discrepancy Dec 22, 2016
gopherbot pushed a commit that referenced this issue Dec 22, 2016
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>
@gopherbot
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Jun 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

2 participants