all: update golang.org/x/... #1688
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ire-and-curses
approved these changes
Aug 30, 2019
18 tasks
|
Haven't done a full review yet (hope to finish it today) but I have some questions right away:
|
|
bartekn
approved these changes
Sep 3, 2019
7 tasks
leighmcculloch
added a commit
that referenced
this pull request
Sep 5, 2019
Use the default `net/http` transport which supports http2, instead of using `x/net/http2`. At some point early on we adopted using `x/net/http2` as the http2 transport in our servers. It was probably necessary at the time, I'm not sure, but it's not necessary today unless we need full access to configure the transport. In our case we're initializing the http2 package with an empty config, so that alone suggests we are unnecessarily using that package. @bartekn noticed this while reviewing #1688 (comment) a dependency upgrade required for the shift from Dep to Modules. The `net/http` package uses a vendored version of `x/net/http2` for http2. We only need to use `x/net/http2` directly if we want to customize it in ways that `net/http` doesn't expose, or if we want to use a later version of the http2 transport that has yet to be included in a release of Go. A huge downside of using the package directly though is that if critical fixes are made to it and included in a minor Go release we won't get them unless we manually update this package. The godocs for the [net/http](https://golang.org/pkg/net/http/) package contain more details about the relationship between `net/http` and `x/net/http2`. Close #1695 Summary of changes: - Remove configuration of the http2 transport for Horizon. - Remove configuration of the http2 transport for code shared by Federation, Bifrost, and Bridge. Testing: No tests that we have in the repository will identify any issues with this change, and any tests we could write would be complex and not helpful longterm, so I took a manual approach: 1. I ran Horizon with TLS enabled, with and without this change, and used curl to verify that the HTTP2 upgrade occurs successfully and results in a `HTTP/2 200` response, using this `curl` command: ``` curl -vk --http2 https://localhost:8000 ``` This test was successful. 2. I ran Horizon with TLS enabled, with and without this change, and used [h2spec](https://github.com/summerwind/h2spec) to verify that the server before and after the change has the same spec passes and failures, using this `h2spec` command: ``` h2spec http2 -t -k -h=localhost -p=8000 ``` This test was successful on most runs. There were definitely spec failures, but they were consistent before and after the change. In one run I noticed spec 6.9.3 part 3 (window updates) failed on `net/http`, however on re-runs it didn't fail. Investigating, I found a report in golang/go#31170 that `x/net/http2` may not be handling window updates correctly in some scenarios, and so it is likely that this issue is intermittent and present when using either and I just witnessed it only on one. An identical change was also made to code shared by the federation, bifrost, and bridge servers. I ran the same tests above against the federation server with TLS enabled to verify that both code paths were unaffected.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
This PR adds tests for the most critical parts of the new functionality or fixes.I've updated any docs (developer docs,.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
I've updated the relevant CHANGELOG (here for Horizon) ifneeded with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Summary
Update
golang.org/x/...packages for transition to Modules (#1634).golang.org/x/cryptogolang/crypto@7f87c0f...cc06ce4 (36 commits)golang.org/x/netgolang/net@9bc2a33...60506f4 (502 commits)golang.org/x/sysgolang/sys@cc5685c...5da2858 (100 commits)golang.org/x/textgolang/text@1cbadb4...v0.3.2 (133 commits)Goal and scope
To make the transition to Modules possible. Some dependencies at their current version have incompatible relationships with other dependencies we have. Dep was more tolerant to incompatible versions than Modules. Making this change ahead of the PR that switches us to Modules makes that PR a functional no-op.
With some of the other package upgrades I've done I've attempted to be as conservative as possible, however with these
golang.org/xpackages are described as part of the Go project, but just stored separately to the main tree so they can be iterated on outside of Go releases. More can be read about them here. We should be able to assume a certain degree of rigor has been maintained.For these packages I've gone with the versions that Modules by default attempted to retrieve. This makes their diffs largely impossible to meaningfully review, however we don't review changes in the Go standard library when new versions of Go are released, so I think these fall into the same bucket.
As a side note we should be keeping these repositories up to date more often. They are only guaranteed to maintain compatibility with the last two versions of Go, which is Go 1.11 and Go 1.12. The versions we were importing were from a time prior to those releases and while it's unlikely there were issues there's no guarantee they would behave appropriately for the latest releases.
Summary of changes
Known limitations & issues
N/A
What shouldn't be reviewed
N/A