net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake #40213

cbf123 · 2020-07-14T17:48:04Z

The fix for #34978 is not correct, the issue is still present.

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

$ go version
go version go1.13.12 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cfriesen/.cache/go-build"
GOENV="/home/cfriesen/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cfriesen/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/cfriesen/devel/h2bug/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build843802376=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Test-case is here: https://github.com/mikedanese/h2bug

What did you expect to see?

The persistent connection is not cached by http.Transport.

What did you see instead?

The persistent connection is cached by http.Transport.

More

Commit https://go.googlesource.com/go/+/88186e5e232625f9c91d639e0cb90a88c6cf1172 was submitted in order to fix #34978, but it appears to be not quite correct due to how http2/transport.go gets included into http via h2_bundle.go with an "http2" prefix added.

If I run the above testcase using "dlv test", then add a breakpoint in src/net/http/transport.go right after the assignment to isH2DialError, and then run "print pconn.alt", it gives me

(dlv) print pconn.alt
net/http.RoundTripper(golang.org/x/net/http2.erringRoundTripper) {
        err: error(*errors.errorString) *{
                s: "broken pipe",},}

At the same time, "isH2DialError" is "false" when it clearly should be "true" for the bugfix to be correct.

Trying to explicitly assert the types gave the following interesting results:

(dlv) print pconn.alt.(http2.erringRoundTripper)
golang.org/x/net/http2.erringRoundTripper {
        err: error(*errors.errorString) *{
                s: "broken pipe",},}
(dlv) print pconn.alt.(http.http2erringRoundTripper)
Command failed: interface conversion: net/http.RoundTripper is golang.org/x/net/http2.erringRoundTripper, not net/http.http2erringRoundTripper

The text was updated successfully, but these errors were encountered:

mikedanese · 2020-07-14T20:43:00Z

Without rearchitecting the fix for #34978, one option is to change this assert:

go/src/net/http/transport.go

Line 1688 in fa98f46

if e, ok := alt.(http2erringRoundTripper); ok {

To check an interface that both http2erringRoundTripper and http2.erringRoundTripper implement.

networkimprov · 2020-07-15T03:19:10Z

cc @fraenkel

fraenkel · 2020-07-16T02:22:54Z

I can't see a way of fixing this without changing the http2 side to something we can detect on both sides.

gopherbot · 2020-07-17T03:30:48Z

Change https://golang.org/cl/243257 mentions this issue: net/http2: erringRoundTripper can be type asserted

gopherbot · 2020-07-17T03:33:34Z

Change https://golang.org/cl/243258 mentions this issue: net/http: erringRoundTripper is an interface

The http transport added a new interface to detect RoundTrippers that always error. Prior to this, the erringRoundTripper would not be identified as such and a new connection was always created. Updates golang/go#40213 Change-Id: Icc315dcc9ce8ea0db94a1f2c58c6a741675d8962 Reviewed-on: https://go-review.googlesource.com/c/net/+/243257 Reviewed-by: Chris Friesen <cbf123@gmail.com> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com> Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot · 2020-08-22T12:53:37Z

Change https://golang.org/cl/249842 mentions this issue: net/http: update bundled x/net/http2

gopherbot · 2020-08-22T14:24:00Z

Change https://golang.org/cl/249937 mentions this issue: net/http2: add test for erringRoundTripper

Updates x/net/http2 to git rev c89045814202410a2d67ec20ecf177ec77ceae7f http2: perform connection health check https://golang.org/cl/198040 (fixes #31643) http2: use ASCII space trimming for parsing Trailer header https://golang.org/cl/231437 all: update golang.org/x/crypto to v0.0.0-20200622213623-75b288015ac9 https://golang.org/cl/239700 (updates #30965) net/http2: fix erringRoundTripper https://golang.org/cl/243257 (updates #40213) also updates the vendored version of golang.org/x/net as per $ go get golang.org/x/net@c890458142 $ go mod tidy $ go mod vendor $ go generate -run bundle std Change-Id: Iea2473ef086df760144d9656f03a0218eb9da91f Reviewed-on: https://go-review.googlesource.com/c/go/+/249842 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>

gopherbot · 2020-10-13T21:57:36Z

Change https://golang.org/cl/261919 mentions this issue: [release-branch.go1.15] src, net/http: update vendor, h2_bundle.go regeneration

gopherbot · 2020-10-13T22:12:55Z

Change https://golang.org/cl/258540 mentions this issue: [release-branch.go1.15] src, net/http: update vendor, regenerate h2_bundle.go

gopherbot · 2020-10-13T22:35:16Z

Change https://golang.org/cl/258538 mentions this issue: [release-branch.go1.14] src, net/http: update vendor, regenerate h2_bundle.go

KevinWang15 · 2021-03-17T06:24:13Z

This bug is fixed, isn't it?

https://go-review.googlesource.com/c/net/+/243257
https://go-review.googlesource.com/c/go/+/243258

KevinWang15 · 2021-03-17T06:35:16Z

I don't see it in release branches other than go 1.16, is it possible to backport it to go 1.15, go 1.14 by any chance?

Thanks, cc @fraenkel

(This one actually could affect Kubernetes: kubernetes/kubernetes#87615 (comment) Kubernetes 1.20 is still using golang 1.15)

networkimprov · 2021-03-17T07:08:28Z

@gopherbot please backport to 1.15, as Kubernetes is affected.

gopherbot · 2021-03-17T07:08:46Z

Backport issue(s) opened: #45076 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

KevinWang15 · 2021-03-17T08:17:23Z

Any chance for 1.14 as well? 😂 cc @networkimprov

seankhliao · 2021-03-17T08:24:44Z

Only the latest 2 releases (1.15 and 1.16) are supported

liggitt · 2021-03-18T17:38:03Z

@networkimprov thanks for opening #45076, would be good to get this fixed in 1.15 since it can't easily be worked around by callers

networkimprov · 2021-03-18T18:37:31Z

Jordan, you may want to note that on #45076, as I have no influence on backport decisions.

gopherbot · 2021-03-24T02:19:26Z

Change https://golang.org/cl/304309 mentions this issue: [release-branch.go1.15-bundle] net/http2: fix erringRoundTripper

gopherbot · 2021-03-24T02:19:51Z

Change https://golang.org/cl/304210 mentions this issue: [release-branch.go1.15] net/http: fix detection of Roundtrippers that always error

gopherbot · 2021-03-29T15:27:17Z

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

Bring in the change in CL 304309 with: 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 For #45076. Updates #40213. Change-Id: I68d5e1f2394508c9cf8627fb852dd9e906d45016 Reviewed-on: https://go-review.googlesource.com/c/go/+/305489 Trust: Dmitri Shuralyov <dmitshur@golang.org> Trust: Emmanuel Odeke <emmanuel@orijtech.com> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com> TryBot-Result: Go Bot <gobot@golang.org>

… always error CL 220905 added code to identify alternate transports that always error by using http2erringRoundTripper. This does not work when the transport is from another package, e.g., http2.erringRoundTripper. Expose a new method that allow detection of such a RoundTripper. Switch to an interface that is both a RoundTripper and can return the underlying error. Fixes #45076. Updates #40213. Change-Id: I170739857ab9e99dffb5fa55c99b24b23c2f9c54 Reviewed-on: https://go-review.googlesource.com/c/go/+/243258 Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com> Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-on: https://go-review.googlesource.com/c/go/+/304210 Trust: Dmitri Shuralyov <dmitshur@golang.org> Trust: Emmanuel Odeke <emmanuel@orijtech.com> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

The net/http transport code is currently broken, it keeps broken persistent connections in the cache if a write error happens during h2 handshake. This is documented in the upstream bug at: golang/go#40213 The problem occurs because in the "go" compiler the http2 code is imported into http as a bundle, with an additional "http2" prefix applied. This messes up the erringRoundTripper handling because the name doesn't match. The solution is to have the "go" compiler look for an interface instead, so we add a new dummy function that doesn't actually do anything and then the "go" compiler can check whether the specified RoundTripper implements the dummy function. Specifically for Kubernetes we need to update the http2 code in the "vendor" subdirectory. A separate change is being made in the "go" compiler. Partial-Bug: 1887438 Depends-On: https://review.opendev.org/c/starlingx/compile/+/780669 Signed-off-by: Chris Friesen <chris.friesen@windriver.com> Change-Id: I95dcbda879973524cd23b2a374537a675ce9435f

The net/http transport code is currently broken, it keeps broken persistent connections in the cache if a write error happens during h2 handshake. This is documented in the upstream bug at: golang/go#40213 The problem occurs because in the "go" compiler the http2 code is imported into http as a bundle, with an additional "http2" prefix applied. This messes up the erringRoundTripper handling because the name doesn't match. The solution is to have the "go" compiler look for an interface instead, so we add a new dummy function that doesn't actually do anything and then the "go" compiler can check whether the specified RoundTripper implements the dummy function. This is slightly different from the proposed upstream fixes for the above upstream bug, it more closely follows how the equivalent problem was solved by IsHTTP2NoCachedConnError(). Change-Id: Ia6e91acb15ff4fe996c8ea9b8a1032cede6c4aab Partial-Bug: 1887438 Signed-off-by: Chris Friesen <chris.friesen@windriver.com>

agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 15, 2020

agnivade added this to the Backlog milestone Jul 15, 2020

cbf123 mentioned this issue Jul 20, 2020

network glitch can lead to node stuck in "NotReady" state kubernetes/kubernetes#93268

Closed

dsymonds assigned bradfitz Jul 25, 2020

dsymonds modified the milestones: Backlog, Go1.16 Jul 25, 2020

gopherbot closed this as completed in 8381408 Aug 25, 2020

KevinWang15 mentioned this issue Mar 17, 2021

(1.17) Kubelet won't reconnect to Apiserver after NIC failure (use of closed network connection) kubernetes/kubernetes#87615

Closed

gopherbot mentioned this issue Mar 17, 2021

net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake [1.15 backport] #45076

Closed

dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 24, 2021

golang locked and limited conversation to collaborators Mar 29, 2022

gopherbot added the FrozenDueToAge label Mar 29, 2022

rsc unassigned bradfitz Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake #40213

net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake #40213

cbf123 commented Jul 14, 2020 •

edited

mikedanese commented Jul 14, 2020

networkimprov commented Jul 15, 2020

fraenkel commented Jul 16, 2020

gopherbot commented Jul 17, 2020

gopherbot commented Jul 17, 2020

gopherbot commented Aug 22, 2020

gopherbot commented Aug 22, 2020

gopherbot commented Oct 13, 2020

gopherbot commented Oct 13, 2020

gopherbot commented Oct 13, 2020

KevinWang15 commented Mar 17, 2021

KevinWang15 commented Mar 17, 2021 •

edited

networkimprov commented Mar 17, 2021

gopherbot commented Mar 17, 2021

KevinWang15 commented Mar 17, 2021 •

edited

seankhliao commented Mar 17, 2021

liggitt commented Mar 18, 2021

networkimprov commented Mar 18, 2021

gopherbot commented Mar 24, 2021

gopherbot commented Mar 24, 2021

gopherbot commented Mar 29, 2021

net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake #40213

net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake #40213

Comments

cbf123 commented Jul 14, 2020 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

More

mikedanese commented Jul 14, 2020

networkimprov commented Jul 15, 2020

fraenkel commented Jul 16, 2020

gopherbot commented Jul 17, 2020

gopherbot commented Jul 17, 2020

gopherbot commented Aug 22, 2020

gopherbot commented Aug 22, 2020

gopherbot commented Oct 13, 2020

gopherbot commented Oct 13, 2020

gopherbot commented Oct 13, 2020

KevinWang15 commented Mar 17, 2021

KevinWang15 commented Mar 17, 2021 • edited

networkimprov commented Mar 17, 2021

gopherbot commented Mar 17, 2021

KevinWang15 commented Mar 17, 2021 • edited

seankhliao commented Mar 17, 2021

liggitt commented Mar 18, 2021

networkimprov commented Mar 18, 2021

gopherbot commented Mar 24, 2021

gopherbot commented Mar 24, 2021

gopherbot commented Mar 29, 2021

cbf123 commented Jul 14, 2020 •

edited

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

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

KevinWang15 commented Mar 17, 2021 •

edited

KevinWang15 commented Mar 17, 2021 •

edited