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

x/net/http2: http2 healthcheck not in h2_bundle.go #41375

Closed
phiphi282 opened this issue Sep 14, 2020 · 8 comments
Closed

x/net/http2: http2 healthcheck not in h2_bundle.go #41375

phiphi282 opened this issue Sep 14, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@phiphi282
Copy link

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

$ go version go1.15.1

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
not important

What did you do?

Inspected the h2_bundle.go to see how it was generated. Error not really reproducible with code.
Inspected this file: https://github.com/golang/go/blob/go1.15.2/src/net/http/h2_bundle.go

What did you expect to see?

Expected commit golang/net@0ba52f6 to be bundled into 1.15.x because the commit exists in the release-branch.go1.15 branch.

What did you see instead?

The commit apparently didn't get bundled into 1.15 release.

@AlexRouSg
Copy link
Contributor

You are mistaking the golang/net branch to mean it's included in the golang/go standard library. golang/net refers to the golang.org/x/net package

For golang/go there is a commit pulling in the changes but it was done after 1.15 and was not backorted so it looks like it will be in the standard library in 1.16 3d77461

@phiphi282
Copy link
Author

I understood the the golang/net repo refers to golang.org/x/net but I am not yet sure about what the branches are for. From the name release-branch.go1.x it sound very much like it is the branch used in the respective minor version.

Otherwise I don't know what the branches are for.

Additionally there is even a reference to the commit I was asking about in the go.mod update commit https://go-review.googlesource.com/c/net/+/198040/.

@AlexRouSg
Copy link
Contributor

cc @odeke-em for questions relating to golang/net

@toothrot toothrot changed the title x/net/http2 http2 healthcheck not in h2_bundle.go x/net/http2: http2 healthcheck not in h2_bundle.go Sep 15, 2020
@toothrot
Copy link
Contributor

The x/ repo branches represent what exists in the Go repo go.mod at the time of cutting a release. This is necessary for us to be able to backport issues to x repositories during a release.

Bundles are a different story. I am not sure why h2_bundle.go wasn't updated when the net repo was most recently updated in the Go 1.15 cycle in https://golang.org/cl/241257, or when it is supposed to be.

Perhaps @bcmills, @odeke-em or @dmitshur knows more, as they have recently discussed this in the commit that @phiphi282 mentioned.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 15, 2020
@toothrot toothrot added this to the Unreleased milestone Sep 15, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 15, 2020

My understanding is that the bundled version and vendored version are currently specified independently. The bundled version wasn't updated to the same version as in src/go.mod for the Go 1.15 release, which is why that particular change isn't there.

I think this issue still exists on the main branch, I filed #41409 for that. We probably need to get to a resolution there before thinking about what, if anything, should to be done for 1.15.

@dmitshur
Copy link
Contributor

I've confirmed this happened on the 1.15 release branch because we have had flexibility between what version of x/net module is vendored, and what version of packages from x/net are bundled. Different versions were selected in the past.

This was covered in #41409 and our plan is to remove this flexibility and add a test to catch regressions. Once that issue is resolved, this shouldn't happen in future releases.

As I understand, this issue was reported because the bundle was manually inspected and this version skew was found to be unexpected. I expect that unless we find evidence that this is causing a problem (I'm not aware of this being the case now), we can just leave the currently selected versions as is.

In either case, thanks for this report.

@phiphi282
Copy link
Author

It didn't cause any serious problems for me. I just found out that some functionality is missing that I expected to be there.
As long as this doesn't happen anymore in future release I am happy 😄

I guess I can close this issue then.

@dmitshur
Copy link
Contributor

dmitshur commented Oct 21, 2020

For the record, on release-branch.go1.15 at the time of this posting, the bundled version of x/net was last updated in CL 231457, and it is currently version v0.0.0-20200501053045-e0ff5e5a1de5.

So, as is, there is a non-zero diff after regenerating net/http:

$ git switch release-branch.go.1.15
$ go generate ./net/http
$ git status | grep bundle
	modified:   net/http/h2_bundle.go
$

Using the aforementioned x/net version with a module-aware bundle results in a zero diff:

$ git switch release-branch.go.1.15
$ go mod edit -replace=golang.org/x/net=golang.org/x/net@v0.0.0-20200501053045-e0ff5e5a1de5
$ GOFLAGS='-mod=mod' go generate ./net/http
$ git status | grep bundle
$

In order to be able to backport individual fixes to it (as needed for #41387 at this time), I've created a release-branch.go1.15-bundle branch in x/net. It initially pointed to commit golang/net@e0ff5e5a1de5.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants