Skip to content

net/http: race in http2Transport [1.14 backport] #42586

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.

Sign up for GitHub

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

Jump to bottom
Closed
dmitshur opened this issue Nov 13, 2020 · 11 comments
Closed

net/http: race in http2Transport [1.14 backport] #42586

dmitshur opened this issue Nov 13, 2020 · 11 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.14.15

Comments

@dmitshur
Copy link
Contributor

@answer1991 requested issue #31192 to be considered for backport to the next 1.14 minor release.

Kubernetes team had already announced GOAWAY feature in Kubernetes v1.19 release notes. However, if Kubernetes enable GOAWAY feature, the client will receive unexpected 500 status code errors which I described at #41234, and there is no workaround. I think backport is necessary.

@gopherbot ple​ase backport, as Kubernetes is affected and there is no workaround.

(The corresponding 1.15 backport request is #42539.)
@dmitshur dmitshur added the CherryPickCandidate Used during the release process for point releases label Nov 13, 2020
@dmitshur dmitshur added this to the Go1.14.13 milestone Nov 13, 2020
@dmitshur dmitshur mentioned this issue Nov 13, 2020
Closed
@cagedmantis cagedmantis modified the milestones: Go1.14.13, Go1.14.14 Dec 3, 2020
@dmitshur dmitshur mentioned this issue Dec 30, 2020
Closed
@answer1991
Copy link

We should also backport #42498.

@cagedmantis
Copy link
Contributor

We are taking a little more time to review this before deciding.

@dmitshur dmitshur modified the milestones: Go1.14.14, Go1.14.15 Jan 19, 2021
@toothrot
Copy link
Contributor

This is a serious issue with no workaround. Approved.

@toothrot toothrot added the CherryPickApproved Used during the release process for point releases label Jan 26, 2021
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Jan 26, 2021
@dmitshur
Copy link
Contributor Author

To resolve this backport issue, CL 253259 (for #31192) and CL 269058 (for #42498) will need to be backported to the x/net's release-branch.go1.14 branch, and then the bundled copy of golang.org/x/net/http2 in net/http needs to be updated. The process is described in https://golang.org/wiki/MinorReleases#making-cherry-pick-cls.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/288114 mentions this issue: [release-branch.go1.14] http2: send a nil error if we cancel a delayed body write

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/288113 mentions this issue: [release-branch.go1.14] net/http2: wait until the request body has been written

gopherbot pushed a commit to golang/net that referenced this issue Jan 29, 2021
@fraenkel @dmitshur
[release-branch.go1.14] http2: wait until the request body has been w…
9c81719 
…ritten

When the clientConn is done with a request, if there is a request body,
we must wait until the body is written otherwise there can be a race
when attempting to rewind the body.

Updates golang/go#42586

Change-Id: I77606cc19372eea8bbd8995102354f092b8042be
Reviewed-on: https://go-review.googlesource.com/c/net/+/253259
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
(cherry picked from commit ff519b6)
Reviewed-on: https://go-review.googlesource.com/c/net/+/288113
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Damien Neil <dneil@google.com>
@gopherbot
Copy link
Contributor

Closed by merging 9c8171985b644dfada08604cbfdaee0c40f4b536 to release-branch.go1.14.

gopherbot pushed a commit to golang/net that referenced this issue Jan 29, 2021
@fraenkel @dmitshur
[release-branch.go1.14] http2: send a nil error if we cancel a delaye…
4acb789 
…d body write

Once a request body is scheduled to be written, a result of the write is always
expected. If the body writer is cancelled, and the write was never started,
send a successful result.

The test included is a modified version of the TestNoSniffExpectRequestBody_h2 found
in net/http.

Updates golang/go#42586

Change-Id: If3f23993170bdf10e9ae4244ec13ae269bd3877a
Reviewed-on: https://go-review.googlesource.com/c/net/+/269058
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 5d6afe9)
Reviewed-on: https://go-review.googlesource.com/c/net/+/288114
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor Author

(This was closed early due to #29599.)

Reopening for updating the vendored/bundled version in the the main repository.

@dmitshur dmitshur reopened this Jan 29, 2021
@gopherbot
Copy link
Contributor

Closed by merging 4acb7895a0574887276b79d6c959e9aa3182386d to release-branch.go1.14.

@dmitshur dmitshur reopened this Jan 29, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/288115 mentions this issue: [release-branch.go1.14] net/http: update bundled x/net/http2

gopherbot pushed a commit that referenced this issue Feb 1, 2021
@neild @dmitshur
[release-branch.go1.14] net/http: update bundled x/net/http2
a2e2011 
Updates bundled http2 to x/net git rev 4acb7895a for:

	http2: send a nil error if we cancel a delayed body write
	https://golang.org/cl/288114

	http2: wait until the request body has been written
	https://golang.org/cl/288113

Created by:

go get -d golang.org/x/net@release-branch.go1.14
go mod tidy
go mod vendor
go generate -run=bundle std

Fixes #42586.

Change-Id: Ib5aecaeb1deb13b8f0e51a864b60eede248aef0d
Reviewed-on: https://go-review.googlesource.com/c/go/+/288115
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 1, 2021

Closed by merging a2e2011 to release-branch.go1.14.

@dmitshur dmitshur closed this as completed Feb 1, 2021
@golang golang locked and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.14.15
Development

No branches or pull requests
5 participants
@toothrot @cagedmantis @dmitshur @answer1991 @gopherbot