go: net/http: transport caches permanently broken persistent connections if write error happens during h2 handshake
The fix for https://github.com/golang/go/issues/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 https://github.com/golang/go/issues/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
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 21 (4 by maintainers)
Commits related to this issue
- net/http2: fix erringRoundTripper 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 c... — committed to golang/net by fraenkel 4 years ago
- src/go.mod, net/http: update bundled and latest golang.org/x/net Updates x/net/http2 to git rev c89045814202410a2d67ec20ecf177ec77ceae7f http2: perform connection health check https://golang... — committed to golang/go by odeke-em 4 years ago
- [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='-mo... — committed to golang/go by dmitshur 3 years ago
- [release-branch.go1.15] net/http: fix detection of Roundtrippers that always error CL 220905 added code to identify alternate transports that always error by using http2erringRoundTripper. This does ... — committed to golang/go by fraenkel 4 years ago
- fix net/http caching of broken persistent connections The net/http transport code is currently broken, it keeps broken persistent connections in the cache if a write error happens during h2 handshake... — committed to starlingx/integ by cbf123 4 years ago
- fix net/http caching of broken persistent connections The net/http transport code is currently broken, it keeps broken persistent connections in the cache if a write error happens during h2 handshake... — committed to starlingx/compile by cbf123 4 years ago
Only the latest 2 releases (1.15 and 1.16) are supported
I can’t see a way of fixing this without changing the http2 side to something we can detect on both sides.
@gopherbot please backport to 1.15, as Kubernetes is affected.