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

[gopherbot]

@networkimprov requested issue #40213 to be considered for backport to the next 1.15 minor release.

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

[liggitt]

it would be good to get this fixed in 1.15 since it can't easily be worked around by callers

[caesarxuchao]

cc @fraenkel

[fraenkel]

I cannot do most of what needs to be done here.
One CL (https://go-review.googlesource.com/c/net/+/243257/) must be merged back into the 1.15 branch for net/http2.
One CL (https://go-review.googlesource.com/c/go/+/243258/) must be merged into net/http 1.15.
Another CL to pick up the new h2_bundle for 1.15.
Another CL (https://go-review.googlesource.com/c/net/+/249937/) needs to be merged back into 1.15 for net/http2

The ordering matters.

[networkimprov]

Michael, is that due to time constraints? If so, maybe Emmanuel can help? @odeke-em

[fraenkel]

I actually no longer understand how net/http2 is being bundled into go 1.15.
The bundle was created from release-branch.go1.15-bundle, and the go.mod for 1.15 points to release-branch.go1.15.

[networkimprov]

cc @dmitshur @toothrot @bradfitz

[dmitshur]

@fraenkel For Go 1.15 backports, the bundled and go.mod versions are distinct, so need to use net’s release-branch.go1.15-bundle for bundle updates, otherwise release-branch.go1.15 for vendor and go.mod. See command sequence in ef3039e for an example.

(The vendored and bundled versions are in sync in Go 1.16 and newer with a test to catch any deviations.)

[fraenkel]

This is going to take some time. The moment I add the fixes for go1.15, the infamous TestDontCacheBrokenHTTP2Conn fails 100% of the time.

[fraenkel]

@dmitshur Those steps don't quite work for me.
I had to do a local replace since the fix is not in github.
Then the bundle command injects net/http as an import. I managed to work around that for my testing purposes with -dst to at least test things.
Since I made my change against release-branch.go1.15-bundle since that made the most sense to me, I cannot mail the code review because there are 5 other fixes (all before mine) which are considered pending.
So what is the magical steps I am missing to get this working right.

[dmitshur]

I had to do a local replace since the fix is not in github.

@fraenkel Yes, those instructions are the final version meant to be used after the backport CL to the x/net bundle branch is submitted, but a local replace is needed for local testing/development until then.

Then the bundle command injects net/http as an import. I managed to work around that for my testing purposes with -dst to at least test things.

There's a chance this is related to issue #44079.

Since I made my change against release-branch.go1.15-bundle since that made the most sense to me, I cannot mail the code review because there are 5 other fixes (all before mine) which are considered pending.

I don't know what 5 other fixes you're referring to, can you please clarify? I would expect you should be able to rebase/cherry-pick what is needed on top of the relevant branches in their current state, nothing else should be "in front".

[fraenkel]

@dmitshur When I cherry-pick the fix on the go1.15-bundle branch:

$ git codereview mail
git-codereview: cannot mail: multiple changes pending; must specify commit on command line; use HEAD to mail all pending changes:
	be5b7b0 [release-branch.go1.15-bundle] http2: fix erringRoundTripper
	16c2bbf [release-branch.go1.15-bundle] http2: send a nil error if we cancel a delayed body write
	2ac096c [release-branch.go1.15-bundle] http2: wait until the request body has been written
	0704177 [release-branch.go1.15-bundle] http2: close Transport connection on write errors
	abf26a1 [release-branch.go1.15-bundle] net/http2: send WINDOW_UPDATE on a body's write failure
	d64cff4 [release-branch.go1.15-bundle] icmp, webdav/internal/xml: avoid string(int)

[dmitshur]

@fraenkel The upstream branch may be misconfigured. What does git rev-parse --abbrev-ref HEAD@{u} print? If it's a remote branch other than origin/release-branch.go1.15-bundle, that means the git codereview mail command was trying to mail those commits to the wrong branch. If that was the problem, run git branch --set-upstream-to=origin/release-branch.go1.15-bundle and then git codereview mail should do the right thing. (This is covered in git-codereview documentation.)

[fraenkel]

@dmitshur That was it. Thanks.

It seems https://go-review.googlesource.com/c/net/+/249937/ has not been merged, so I won't include the test.

[gopherbot]

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

[gopherbot]

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

[dmitshur]

Approving per discussion in a release meeting. Only Go 1.15 needs this backport.

[gopherbot]

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

[gopherbot]

Closed by merging be87281 to release-branch.go1.15.