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

Jump to bottom

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

Closed
gopherbot opened this issue Mar 17, 2021 · 19 comments
Closed

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

gopherbot opened this issue Mar 17, 2021 · 19 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.15.11

Comments

@gopherbot
Copy link

@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.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Mar 17, 2021
@gopherbot gopherbot mentioned this issue Mar 17, 2021
Closed
@gopherbot gopherbot added this to the Go1.15.11 milestone Mar 17, 2021
@liggitt
Copy link
Contributor

liggitt commented Mar 18, 2021

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

@caesarxuchao
Copy link

cc @fraenkel

@fraenkel
Copy link
Contributor

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
Copy link

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

@fraenkel
Copy link
Contributor

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
Copy link

cc @dmitshur @toothrot @bradfitz

@dmitshur
Copy link
Contributor

dmitshur commented Mar 20, 2021 •
edited

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

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

@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.

@caesarxuchao caesarxuchao mentioned this issue Mar 21, 2021
Closed
@dmitshur
Copy link
Contributor

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

fraenkel commented Mar 23, 2021 •
edited

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

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

@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
Copy link
Author

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

@gopherbot
Copy link
Author

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

@dmitshur
Copy link
Contributor

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

@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Mar 25, 2021
gopherbot pushed a commit to golang/net that referenced this issue Mar 29, 2021
@fraenkel @dmitshur
[release-branch.go1.15-bundle] net/http2: fix erringRoundTripper
422a156 
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#45076

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>
Reviewed-on: https://go-review.googlesource.com/c/net/+/304309
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@gopherbot
Copy link
Author

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

@gopherbot

This comment has been minimized.

Sign in to view
@gopherbot
Copy link
Author

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

gopherbot pushed a commit that referenced this issue Mar 29, 2021
@dmitshur
[release-branch.go1.15] net/http: update bundled x/net/http2
69b9431 
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>
gopherbot pushed a commit that referenced this issue Mar 29, 2021
@fraenkel @dmitshur
[release-branch.go1.15] net/http: fix detection of Roundtrippers that…
be87281 
… 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>
@knz knz mentioned this issue Apr 19, 2021
Closed
@rickystewart rickystewart mentioned this issue Apr 19, 2021
Merged
11 tasks
rickystewart added a commit to rickystewart/cockroach that referenced this issue Apr 19, 2021
@rickystewart
build: upgrade go to 1.15.11
3d03033 
Pick up fixes to the following Go bugs:

* golang/go#45076
* golang/go#45187
* golang/go#42884

* [ ] Adjust the Pebble tests to run in new version.
* [x] Adjust version in Docker image ([source](./builder/Dockerfile)).
* [x] Rebuild and push the Docker image (following [Basic Process](#basic-process))
* [x] Bump the version in `WORKSPACE` under `go_register_toolchains`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases).
* [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)).
* [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release.
* [ ] Bump the go version in `go.mod`. You may also need to rerun `make vendor_rebuild` if vendoring has changed.
* [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh#L40-42)).
* [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`).
* [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects.
* [x] Adjust `GO_VERSION` in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh))
  and ask the Developer Infrastructure team to deploy new images.

Resolves cockroachdb#63836.

Release note (general change): Upgrade the CRDB binary to Go 1.15.10
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Apr 19, 2021
@postamar @knz @rickystewart
Merge #63785 #63787 #63839 #63853 #63857
0609f89 
63785: sql: use catalog.Mutation, catalog.Column and catalog.Index where possible r=postamar a=postamar

    sql: use catalog.Mutation where possible
    
    This commit is a refactor which introduces catalog.Mutation wherever
    possible, in an effort to avoid using the descpb.DescriptorMutation
    type directly.
    
    Release note: None


    sql: use catalog.Column and catalog.Index where possible
    
    This commit is a refactor which introduces the catalog.Column and
    catalog.Index interfaces wherever possible. These had recently been
    added in an effort to avoid using the descpb.ColumnDescriptor and
    descpb.IndexDescriptor types directly. This commit generalizes their
    usage throughout the sql packages.
    
    Fixes #63755.
    
    Release note: None



63787: roachtest: added hibernate ignorelist for 21.1 r=rafiss a=mnovelodou

Previously, some random issues with hibernate were afecting roachtest
This was inadequate because they are false failures
To address this, this patch set these tests in ignore list

Release note: none

63839: cli: bump the cobra dependency and add autocompletion for fish r=stevendanna a=knz

Fixes  #63838 . 
Fixes  #50187.

This updates the cobra dep which hadn't been updated since 2017.

Also picks up the new support for fish autocompletions.

Release note (cli change): Certain errors caused by invalid
command-line arguments are now printed on the process' standard
error stream, instead of standard output.

Release note (cli change): The `cockroach gen autocomplete` command
has been updated and can now produce autocompletion definitions
for the `fish` shell.

63853: build: upgrade go to 1.15.11 r=rail a=rickystewart

Pick up fixes to the following Go bugs:

* golang/go#45076
* golang/go#45187
* golang/go#42884

* [ ] Adjust the Pebble tests to run in new version.
* [x] Adjust version in Docker image ([source](./builder/Dockerfile)).
* [x] Rebuild and push the Docker image (following [Basic Process](#basic-process))
* [x] Bump the version in `WORKSPACE` under `go_register_toolchains`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases).
* [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)).
* [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release.
* [ ] Bump the go version in `go.mod`. You may also need to rerun `make vendor_rebuild` if vendoring has changed.
* [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh#L40-42)).
* [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`).
* [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects.
* [x] Adjust `GO_VERSION` in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh))
  and ask the Developer Infrastructure team to deploy new images.

Resolves #63836.

Release note (general change): Upgrade the CRDB binary to Go 1.15.10

63857: changefeedccl: Make `kafka_sink_config` a valid option. r=stevendanna a=miretskiy

Add `kafka_sink_config` to the list of allowed changefeed options.

Release Notes: None

Co-authored-by: Marius Posta <marius@cockroachlabs.com>
Co-authored-by: MiguelNovelo <miguel.novelo@digitalonus.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
rickystewart added a commit to rickystewart/cockroach that referenced this issue Apr 19, 2021
@rickystewart
build: upgrade go to 1.15.11
564b086 
Pick up fixes to the following Go bugs:

* golang/go#45076
* golang/go#45187
* golang/go#42884

* [ ] Adjust the Pebble tests to run in new version.
* [x] Adjust version in Docker image ([source](./builder/Dockerfile)).
* [x] Rebuild and push the Docker image (following [Basic Process](#basic-process))
* [x] Bump the version in `WORKSPACE` under `go_register_toolchains`. You may need to bump [rules_go](https://github.com/bazelbuild/rules_go/releases).
* [x] Bump the version in `builder.sh` accordingly ([source](./builder.sh#L6)).
* [ ] Bump the version in `go-version-check.sh` ([source](./go-version-check.sh)), unless bumping to a new patch release.
* [ ] Bump the go version in `go.mod`. You may also need to rerun `make vendor_rebuild` if vendoring has changed.
* [x] Bump the default installed version of Go in `bootstrap-debian.sh` ([source](./bootstrap/bootstrap-debian.sh#L40-42)).
* [x] Replace other mentions of the older version of go (grep for `golang:<old_version>` and `go<old_version>`).
* [ ] Update the `builder.dockerImage` parameter in the TeamCity [`Cockroach`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Cockroach&tab=projectParams) and [`Internal`](https://teamcity.cockroachdb.com/admin/editProject.html?projectId=Internal&tab=projectParams) projects.
* [x] Adjust `GO_VERSION` in the TeamCity agent image ([setup script](./packer/teamcity-agent.sh))
  and ask the Developer Infrastructure team to deploy new images.

Resolves cockroachdb#63836.

Release note (general change): Upgrade the CRDB binary to Go 1.15.10
@rickystewart rickystewart mentioned this issue Apr 19, 2021
Merged
11 tasks
@knz knz mentioned this issue Apr 22, 2021
Closed
jseldess pushed a commit to cockroachdb/docs that referenced this issue May 4, 2021
Update Go version requirement for building 21.1 from source
094d7cd 
Technically, the minimum required is 1.15.3:
https://github.com/cockroachdb/cockroach/blob/release-21.1/build/go-version-check.sh#L10

However, 1.15.11 avoids these Go bugs:
golang/go#45076
golang/go#45187
golang/go#42884

So to prevent users from running into those, we listing
1.15.11 as the minimum required version, followed by
a note that you can use IGNORE_GOVERS=1 to try building
with older versions.

Fixes #10468
Fixes #9081
@jseldess jseldess mentioned this issue May 4, 2021
Merged
@golang golang locked and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.15.11
Development

No branches or pull requests
6 participants
@networkimprov @fraenkel @liggitt @dmitshur @caesarxuchao @gopherbot