Skip to content
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: transport caches permanently broken persistent connections if write error happens during h2 handshake #40213

Open
cbf123 opened this issue Jul 14, 2020 · 5 comments
Assignees
Milestone

Comments

@cbf123
Copy link

@cbf123 cbf123 commented Jul 14, 2020

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
@mikedanese
Copy link
Contributor

@mikedanese mikedanese commented Jul 14, 2020

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

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

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

@networkimprov
Copy link

@networkimprov networkimprov commented Jul 15, 2020

@agnivade agnivade added this to the Backlog milestone Jul 15, 2020
@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Jul 16, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 17, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 17, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.