Skip to content

/ 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

Jump to bottom

net/http: race in http2Transport [1.15 backport] #42539

Closed
gopherbot opened this issue Nov 12, 2020 · 11 comments
Closed

net/http: race in http2Transport [1.15 backport] #42539

gopherbot opened this issue Nov 12, 2020 · 11 comments
Labels
CherryPickApproved
Milestone
Go1.15.8

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 12, 2020

@answer1991 requested issue #31192 to be considered for backport to the next 1.15 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 to golang 1.15 is necessary.

@gopherbot please backport, as Kubernetes is affected and there is no workaround.
@gopherbot gopherbot mentioned this issue Nov 12, 2020
Closed
@gopherbot gopherbot added this to the Go1.15.5 milestone Nov 12, 2020
@cagedmantis cagedmantis modified the milestones: Go1.15.5, Go1.15.6 Nov 12, 2020
@dmitshur dmitshur mentioned this issue Nov 13, 2020
Closed
@cagedmantis cagedmantis modified the milestones: Go1.15.6, Go1.15.7 Dec 3, 2020
@dmitshur dmitshur mentioned this issue Dec 30, 2020
Closed
@answer1991
Copy link

@answer1991 answer1991 commented Jan 1, 2021

We should also backport #42498.
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jan 14, 2021

We are taking a little more time to review this before deciding.
@dmitshur dmitshur modified the milestones: Go1.15.7, Go1.15.8 Jan 19, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Jan 26, 2021

This is a serious issue with no workaround. Approved.
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jan 26, 2021

To resolve this backport issue, first, CL 253259 (for #31192) and CL 269058 (for #42498) will need to be backported to the x/net's release-branch.go1.15-bundle branch.

Afterwards, the bundled copy of golang.org/x/net/http2 in net/http needs to be updated using that x/net branch. See the commit message of f68af19 or ef3039e for a sequence of commands that can be used. Otherwise, the process is described in https://golang.org/wiki/MinorReleases#making-cherry-pick-cls.
@answer1991
Copy link

@answer1991 answer1991 commented Jan 28, 2021

I have no permission to cherry-pick #31192 and #42498 to x/net's 1.15 release branch, should I ask the author to do that?
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 29, 2021

Change https://golang.org/cl/287992 mentions this issue: [release-branch.go1.15] net/http2: wait until the request body has been written
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 29, 2021

Change https://golang.org/cl/287993 mentions this issue: [release-branch.go1.15] http2: send a nil error if we cancel a delayed body write
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 29, 2021

Change https://golang.org/cl/288013 mentions this issue: [release-branch.go1.15-bundle] http2: send a nil error if we cancel a delayed body write
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 29, 2021

Change https://golang.org/cl/288012 mentions this issue: [release-branch.go1.15-bundle] 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.15-bundle] http2: wait until the request body has…
2ac096c 
… been written

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#42539

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/+/288012
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Jan 29, 2021
@fraenkel @dmitshur
[release-branch.go1.15-bundle] http2: send a nil error if we cancel a…
16c2bbf 
… delayed 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#42539

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/+/288013
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jan 29, 2021

Change https://golang.org/cl/288112 mentions this issue: [release-branch.go1.15] net/http: update bundled x/net/http2
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 1, 2021

Closed by merging a01db0d to release-branch.go1.15.
@gopherbot gopherbot closed this Feb 1, 2021
gopherbot pushed a commit that referenced this issue Feb 1, 2021
@neild @dmitshur
[release-branch.go1.15] net/http: update bundled x/net/http2
a01db0d 
Updates bundled http2 to x/net git rev 16c2bbf55 for:

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

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

Created by:

go mod edit -replace=golang.org/x/net=golang.org/x/net@release-branch.go1.15-bundle
GOFLAGS='-mod=mod' go generate -run=bundle std
go mod edit -dropreplace=golang.org/x/net
go get -d golang.org/x/net@release-branch.go1.15
go mod tidy
go mod vendor

Fixes #42539

Change-Id: I299c6d4a67ebc036e45c978e4d03cba73717b363
Reviewed-on: https://go-review.googlesource.com/c/go/+/288112
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved
Projects
None yet
Milestone
Go1.15.8
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
@toothrot @cagedmantis @dmitshur @answer1991 @gopherbot