/go

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: first byte of outgoing request body is sometimes lost #17071

Closed
jacobsa opened this Issue Sep 12, 2016 · 27 comments

Comments

Assignees

@bradfitz bradfitz

Labels
Projects
None yet
Milestone
  Go1.7.3
5 participants
@jacobsa @bradfitz @gopherbot @gaillard @broady
@jacobsa
Contributor

jacobsa commented Sep 12, 2016 •
edited
Edited 1 time
  • @jacobsa jacobsa edited Sep 12, 2016 (most recent) 

% go version
go version devel +8086e7c Mon Sep 12 06:12:35 2016 +0000 darwin/amd64

(Apologies for not having been able to reproduce this hermetically. I suspect maybe it has something to do with SSL, which I don't know how to set up locally. I'm pretty sure the bug does lie in the Go client though; please keep reading.)

Here is a program that makes a bunch of calls to the Google Cloud Storage API. The calls are all identical. Although this is a heavily-reduced test case and they are not valid according to the GCS API (in particular they're missing credentials), they all do supply valid JSON in the request body.

Despite this, every time I run the program, after at most a few tens of seconds I receive an HTTP 400 parse error from GCS:

% go run http2_bug.go
[...]
Req body:
{"bucket":"foo","name":"bar"}
[...]
Resp body:
{
 "error": {
  "errors": [
   {
    "domain": "global",
    "reason": "parseError",
    "message": "Parse Error"
   }
  ],
  "code": 400,
  "message": "Parse Error"
 }
}

Status code: 400

I happen to have insider knowledge that this charmingly terse error message from GCS indicates that the JSON in the request body could not be parsed. (If you want details, please contact me internally within Google.) In particular, the server on the other end does not receive the opening { character.

Go's own HTTP2 debugging agrees.

% GODEBUG=http2debug=2 go run http2_bug.go

With this invocation, among lots of other output I see

2016/09/12 20:46:54 http2: Framer 0xc420242180: wrote HEADERS flags=END_HEADERS stream=455 len=7
2016/09/12 20:46:54 http2: Framer 0xc420242180: wrote DATA stream=455 len=1 data="\""
2016/09/12 20:46:54 http2: Framer 0xc420242180: wrote DATA stream=455 len=27 data="bucket\":\"foo\",\"name\":\"bar\"}"
2016/09/12 20:46:54 http2: Framer 0xc420242180: wrote DATA flags=END_STREAM stream=455 len=0 data=""
2016/09/12 20:46:54 http2: Framer 0xc420242180: read HEADERS flags=END_HEADERS stream=455 len=94
2016/09/12 20:46:54 http2: Framer 0xc420242180: read DATA flags=END_STREAM stream=455 len=171 data="{\n \"error\": {\n  \"errors\": [\n   {\n    \"domain\": \"global\",\n    \

The http2 package has forgotten to write the opening curly brace. In contrast, everything works fine if I use GODEBUG=http2client=0 to downgrade to HTTP1.

FYI, I have observed this from Sydney (Google office and at home, multiple computers) and from a computer within a Google data center in the US. I've seen this in Go 1.7.1 and Go 1.6.1, as well as at HEAD.

@jacobsa jacobsa changed the title from net/http: first character of outgoing request body is sometimes lost to net/http: first byte of outgoing request body is sometimes lost Sep 12, 2016

@bradfitz bradfitz self-assigned this Sep 12, 2016

@bradfitz bradfitz added this to the Go1.7.2 milestone Sep 12, 2016

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Sep 12, 2016

Member

Well, crap. https://golang.org/cl/29074

Thanks for the report.
Member

bradfitz commented Sep 12, 2016

Well, crap. https://golang.org/cl/29074

Thanks for the report.
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 12, 2016

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

gopherbot commented Sep 12, 2016

CL https://golang.org/cl/29074 mentions this issue.
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Sep 12, 2016

Member

This probably warrants a Go 1.7.2 since it's sporadic data corruption. It happens whenever a Request.ContentLength == 0 && Request.Body != nil && the connection is either closed or temporarily at max-streams capacity.

Two possible workarounds, though:

  1. set Request.ContentLength
  2. disabled HTTP/2
Member

bradfitz commented Sep 12, 2016

This probably warrants a Go 1.7.2 since it's sporadic data corruption. It happens whenever a Request.ContentLength == 0 && Request.Body != nil && the connection is either closed or temporarily at max-streams capacity.

Two possible workarounds, though:

  1. set Request.ContentLength
  2. disabled HTTP/2

gopherbot pushed a commit to golang/net that referenced this issue Sep 12, 2016

@bradfitz
http2: don't sniff first Request.Body byte in Transport until we have… 
… a conn

bodyAndLength mutates Request.Body if Request.ContentLength == 0,
reading the first byte to determine whether it's actually empty or
just undeclared. But we did that before we checked whether our
connection was overloaded, which meant the caller could retry the
request on an new or lesser-loaded connection, but then lose the first
byte of the request.

Updates golang/go#17071 (needs bundle into std before fixed)

Change-Id: I996a92ad037b45bc49e7cf0801a2027bbbb3c920
Reviewed-on: https://go-review.googlesource.com/29074
Reviewed-by: Gleb Stepanov <glebstepanov1992@gmail.com>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
749a502
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 12, 2016

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

gopherbot commented Sep 12, 2016

CL https://golang.org/cl/29081 mentions this issue.
@jacobsa

This comment has been minimized.

Show comment
Hide comment
@jacobsa

jacobsa Sep 12, 2016

Contributor

Thanks for the quick fix. Please also consider a patch release for Go 1.6. Note that this bug was actually introduced in a patch release, since Go 1.6 didn't have HTTP2 on by default but 406752b made it into Go 1.6.2. (I may be confused about the details.)
Contributor

jacobsa commented Sep 12, 2016 •
edited
Edited 1 time
  • @jacobsa jacobsa edited Sep 12, 2016 (most recent)

Thanks for the quick fix. Please also consider a patch release for Go 1.6. Note that this bug was actually introduced in a patch release, since Go 1.6 didn't have HTTP2 on by default but 406752b made it into Go 1.6.2. (I may be confused about the details.)
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Sep 12, 2016

Member

The first Go 1.6 included HTTP/2 by default for all user-created http.Transports but not for the http.DefaultTransport. Go 1.6.2 fixed that. All the Go 1.6 betas before Go 1.6 had it enabled for the DefaultTransport too.

In any case, we don't issue point releases for two releases back. Only Go 1.7.x at this point.
Member

bradfitz commented Sep 12, 2016

The first Go 1.6 included HTTP/2 by default for all user-created http.Transports but not for the http.DefaultTransport. Go 1.6.2 fixed that. All the Go 1.6 betas before Go 1.6 had it enabled for the DefaultTransport too.

In any case, we don't issue point releases for two releases back. Only Go 1.7.x at this point.
@jacobsa

This comment has been minimized.

Show comment
Hide comment
@jacobsa

jacobsa Sep 12, 2016

Contributor

Okay, sounds good. Thanks for clarifying.
Contributor

jacobsa commented Sep 12, 2016

Okay, sounds good. Thanks for clarifying.

@bradfitz bradfitz referenced this issue Sep 13, 2016

Closed

net/http: backport http2 first byte loss CL to go 1.6.x maybe #17091

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 13, 2016

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

gopherbot commented Sep 13, 2016

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

@gopherbot gopherbot closed this in 802cb59 Sep 13, 2016

gopherbot pushed a commit to golang/net that referenced this issue Sep 13, 2016

@bradfitz
[release-branch.go1.7] http2: don't sniff first Request.Body byte in … 
…Transport until we have a conn

bodyAndLength mutates Request.Body if Request.ContentLength == 0,
reading the first byte to determine whether it's actually empty or
just undeclared. But we did that before we checked whether our
connection was overloaded, which meant the caller could retry the
request on an new or lesser-loaded connection, but then lose the first
byte of the request.

Updates golang/go#17071 (needs bundle into std before fixed)

Change-Id: I996a92ad037b45bc49e7cf0801a2027bbbb3c920
Reviewed-on: https://go-review.googlesource.com/29074
Reviewed-by: Gleb Stepanov <glebstepanov1992@gmail.com>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/29081
8d4d01f
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 13, 2016

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

gopherbot commented Sep 13, 2016

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

jacobsa added a commit to jacobsa/gcloud that referenced this issue Sep 14, 2016

@jacobsa
Always be explicit about HTTP request body lengths when possible. 
This works around golang/go#17071 in particular, and is more robust to
future similar issues.

The sniffing magic performed by http.NewRequest has now been hoisted to
the one place where it's actually needed.
17f7008

jacobsa added a commit to jacobsa/comeback that referenced this issue Sep 14, 2016

@jacobsa
Update to jacobsa/gcloud@17f7008. 
In order to work around corruption issues caused by golang/go#17071.
d252af5

gopherbot pushed a commit that referenced this issue Sep 15, 2016

@bradfitz
[release-branch.go1.7] net/http: update bundled http2 
Updates bundled http2 for x/net/http2 git rev 8d4d01f0 for:

[release-branch.go1.7] http2: don't sniff first Request.Body byte in Transport until we have a conn
https://golang.org/cl/29074

Fixes #17071

Change-Id: I37fef5c2c0fdf499545f9af08abd5f9edb2da4c0
Reviewed-on: https://go-review.googlesource.com/29111
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
b67902f
@gaillard

This comment has been minimized.

Show comment
Hide comment
@gaillard

gaillard Oct 14, 2016

Is this going to be released?

gaillard commented Oct 14, 2016

Is this going to be released?
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 14, 2016

Member

Yes. @broady is planning on Monday.
Member

bradfitz commented Oct 14, 2016

Yes. @broady is planning on Monday.
@gaillard

This comment has been minimized.

Show comment
Hide comment
@gaillard

gaillard Oct 17, 2016

@bradfitz Forgot to mention, we had to lob this commit off because it caused a deadlock. Some mutex between RoundTrip and CanTakeNewRequest. Sorry I don't have an exact senario for you.

gaillard commented Oct 17, 2016 •
edited
Edited 1 time
  • @gaillard gaillard edited Oct 17, 2016 (most recent)

@bradfitz Forgot to mention, we had to lob this commit off because it caused a deadlock. Some mutex between RoundTrip and CanTakeNewRequest. Sorry I don't have an exact senario for you.
@broady

This comment has been minimized.

Show comment
Hide comment
@broady

broady Oct 17, 2016

Member

@gaillard sorry, can you elaborate? this commit is bad?
Member

broady commented Oct 17, 2016

@gaillard sorry, can you elaborate? this commit is bad?
@broady

This comment has been minimized.

Show comment
Hide comment
@broady

broady Oct 17, 2016

Member

Holding off the 1.7.2 release announcement to get more information from @gaillard.
Member

broady commented Oct 17, 2016

Holding off the 1.7.2 release announcement to get more information from @gaillard.

@fsouza fsouza referenced this issue Oct 18, 2016

Closed

Go 1.7.2 #6033

@gaillard

This comment has been minimized.

Show comment
Hide comment
@gaillard

gaillard Oct 18, 2016

I don't have much more information. Just that it locked up in the client in a concurrent scenario and appeared to be sitting in those two places from the goroutine dumps. Tested with and without the commit but it was not a fully isolated test however seems highly likely since the commit moved that call past a lock.

gaillard commented Oct 18, 2016

I don't have much more information. Just that it locked up in the client in a concurrent scenario and appeared to be sitting in those two places from the goroutine dumps. Tested with and without the commit but it was not a fully isolated test however seems highly likely since the commit moved that call past a lock.
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 18, 2016

Member

Was your request body an io.Pipe? Sounds like #17480
Member

bradfitz commented Oct 18, 2016

Was your request body an io.Pipe? Sounds like #17480
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 18, 2016

Member

Please post stack traces of the hang if not.
Member

bradfitz commented Oct 18, 2016

Please post stack traces of the hang if not.
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 18, 2016

Member

Okay, better fix: https://go-review.googlesource.com/31326

@broady, we'll need to cherry-pick that to the 1.7 branch of the "net" repo, and then re-run "go generate" from the "go" repo's 1.7 branch.
Member

bradfitz commented Oct 18, 2016

Okay, better fix: https://go-review.googlesource.com/31326

@broady, we'll need to cherry-pick that to the 1.7 branch of the "net" repo, and then re-run "go generate" from the "go" repo's 1.7 branch.
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 18, 2016

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

gopherbot commented Oct 18, 2016

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

gopherbot pushed a commit to golang/net that referenced this issue Oct 18, 2016

@bradfitz
http2: never Read from Request.Body in Transport to determine Content… 
…Length

Updates golang/go#17480 (fixes after vendor to std)
Updates golang/go#17071 (a better fix than the original one)

Change-Id: Ibb49d2cb825e28518e688b5c3e0952956a305050
Reviewed-on: https://go-review.googlesource.com/31326
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
a625e39
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 18, 2016

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

gopherbot commented Oct 18, 2016

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

gopherbot pushed a commit to golang/net that referenced this issue Oct 18, 2016

@bradfitz @broady
[release-branch.go1.7] http2: never Read from Request.Body in Transpo… 
…rt to determine ContentLength

Updates golang/go#17480 (fixes after vendor to std)
Updates golang/go#17071 (a better fix than the original one)

Change-Id: Ibb49d2cb825e28518e688b5c3e0952956a305050
Reviewed-on: https://go-review.googlesource.com/31326
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-on: https://go-review.googlesource.com/31361
Run-TryBot: Chris Broadfoot <cbro@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
d4c55e6
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 18, 2016

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

gopherbot commented Oct 18, 2016

CL https://golang.org/cl/31432 mentions this issue.
@gaillard

This comment has been minimized.

Show comment
Hide comment
@gaillard

gaillard Oct 18, 2016

I do remember a pipe in the stacks, sorry I don't have them anymore.

gaillard commented Oct 18, 2016

I do remember a pipe in the stacks, sorry I don't have them anymore.
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 18, 2016

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

gopherbot commented Oct 18, 2016

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

gopherbot pushed a commit that referenced this issue Oct 18, 2016

@bradfitz
net/http: update test to check Content-Length 0 Body more reliably 
The way to send an explicitly-zero Content-Length is to set a nil Body.

Fix this test to do that, rather than relying on type sniffing.

Updates #17480
Updates #17071

Change-Id: I6a38e20f17013c88ec4ea69d73c507e4ed886947
Reviewed-on: https://go-review.googlesource.com/31434
TryBot-Result: Gobot Gobot <gobot@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
97b0415
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 18, 2016

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

gopherbot commented Oct 18, 2016

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

gopherbot pushed a commit that referenced this issue Oct 18, 2016

@bradfitz @broady
[release-branch.go1.7] net/http: update test to check Content-Length … 
…0 Body more reliably

The way to send an explicitly-zero Content-Length is to set a nil Body.

Fix this test to do that, rather than relying on type sniffing.

Updates #17480
Updates #17071

Change-Id: I6a38e20f17013c88ec4ea69d73c507e4ed886947
Reviewed-on: https://go-review.googlesource.com/31434
TryBot-Result: Gobot Gobot <gobot@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-on: https://go-review.googlesource.com/31437
Run-TryBot: Chris Broadfoot <cbro@golang.org>
740dfba

gopherbot pushed a commit that referenced this issue Oct 19, 2016

@broady
[release-branch.go1.7] net/http: update bundled http2 
Updates bundled http2 for x/net/http2 git rev d4c55e66 for:

[release-branch.go1.7] http2: never Read from Request.Body in Transport
to determine ContentLength
https://golang.org/cl/31361

Updates #17480
Updates #17071

Change-Id: I2231adaed3cb5b368927a9654dcf7e69a8b664b6
Reviewed-on: https://go-review.googlesource.com/31432
Run-TryBot: Chris Broadfoot <cbro@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
3b6f4b0
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 19, 2016

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

gopherbot commented Oct 19, 2016

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

gopherbot pushed a commit that referenced this issue Oct 19, 2016

@bradfitz
net/http: make NewRequest set empty Body nil, don't peek Read Body in… 
… Transport

This CL makes NewRequest set Body nil for known-zero bodies, and makes
the http1 Transport not peek-Read a byte to determine whether there's
a body.

Background:

Many fields of the Request struct have different meanings for whether
they're outgoing (via the Transport) or incoming (via the Server).

For outgoing requests, ContentLength and Body are documented as:

	// Body is the request's body.
	//
	// For client requests a nil body means the request has no
	// body, such as a GET request. The HTTP Client's Transport
	// is responsible for calling the Close method.
	Body io.ReadCloser

	// 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 with a non-nil Body is
	// also treated as unknown.
	ContentLength int64

Because of the ambiguity of what ContentLength==0 means, the http1 and
http2 Transports previously Read the first byte of a non-nil Body when
the ContentLength was 0 to determine whether there was an actual body
(with a non-zero length) and ContentLength just wasn't populated, or
it was actually empty.

That byte-sniff has been problematic and gross (see #17480, #17071)
and was removed for http2 in a previous commit.

That means, however, that users doing:

    req, _ := http.NewRequest("POST", url, strings.NewReader(""))

... would not send a Content-Length header in their http2 request,
because the size of the reader (even though it was known, being one of
the three common recognized types from NewRequest) was zero, and so
the HTTP Transport thought it was simply unset.

To signal explicitly-zero vs unset-zero, this CL changes NewRequest to
signal explicitly-zero by setting the Body to nil, instead of the
strings.NewReader("") or other zero-byte reader.

This CL also removes the byte sniff from the http1 Transport, like
https://golang.org/cl/31326 did for http2.

Updates #17480
Updates #17071

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

@edvakf edvakf referenced this issue Oct 20, 2016

Closed

ベンチマーカー：GoのHTTP2クライアントで最初のバイトがロストするときがあるらしい #230

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 21, 2016

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

gopherbot commented Oct 21, 2016

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

gopherbot pushed a commit that referenced this issue Oct 21, 2016

@bradfitz
net/http: update bundled http2 
Updates http2 to x/net/http2 git rev 40a0a18 for:

    http2: fix Server race with concurrent Read/Close
    http2: make Server reuse 64k request body buffer between requests
    http2: never Read from Request.Body in Transport to determine ContentLength

Fixes #17480
Updates #17071

Change-Id: If142925764a2e148f95957f559637cfc1785ad21
Reviewed-on: https://go-review.googlesource.com/31737
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
a50fbcd

@glycerine glycerine referenced this issue Oct 22, 2016

Closed

is non-http2 transport available? #10150

@jba jba referenced this issue Oct 24, 2016

Closed

googleapi: Error 400: Parse Error, parseError #389

ceseo added a commit to powertechpreview/go that referenced this issue Dec 1, 2016

@bradfitz @ceseo
[release-branch.go1.7] net/http: update bundled http2 
Updates bundled http2 for x/net/http2 git rev 8d4d01f0 for:

[release-branch.go1.7] http2: don't sniff first Request.Body byte in Transport until we have a conn
https://golang.org/cl/29074

Fixes #17071

Change-Id: I37fef5c2c0fdf499545f9af08abd5f9edb2da4c0
Reviewed-on: https://go-review.googlesource.com/29111
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
e254008

ceseo added a commit to powertechpreview/go that referenced this issue Dec 1, 2016

@bradfitz @ceseo
[release-branch.go1.7] net/http: update test to check Content-Length … 
…0 Body more reliably

The way to send an explicitly-zero Content-Length is to set a nil Body.

Fix this test to do that, rather than relying on type sniffing.

Updates #17480
Updates #17071

Change-Id: I6a38e20f17013c88ec4ea69d73c507e4ed886947
Reviewed-on: https://go-review.googlesource.com/31434
TryBot-Result: Gobot Gobot <gobot@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-on: https://go-review.googlesource.com/31437
Run-TryBot: Chris Broadfoot <cbro@golang.org>
d3647ae

ceseo added a commit to powertechpreview/go that referenced this issue Dec 1, 2016

@broady @ceseo
[release-branch.go1.7] net/http: update bundled http2 
Updates bundled http2 for x/net/http2 git rev d4c55e66 for:

[release-branch.go1.7] http2: never Read from Request.Body in Transport
to determine ContentLength
https://golang.org/cl/31361

Updates #17480
Updates #17071

Change-Id: I2231adaed3cb5b368927a9654dcf7e69a8b664b6
Reviewed-on: https://go-review.googlesource.com/31432
Run-TryBot: Chris Broadfoot <cbro@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
a32fab0
@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Dec 22, 2016

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

gopherbot commented Dec 22, 2016

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

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

@bradfitz
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>
6e36811

@bradfitz bradfitz referenced this issue May 5, 2017

Closed

net/http: client do not set Content-Length: 0 with an io.Reader with no bytes. #20257

@golang golang locked and limited conversation to collaborators Dec 22, 2017

@gopherbot gopherbot added the FrozenDueToAge label Dec 22, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.