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

proposal: x/net/http2: Add ConfigureTransportWithOptions function #41721

Closed
caesarxuchao opened this issue Sep 30, 2020 · 6 comments
Closed

proposal: x/net/http2: Add ConfigureTransportWithOptions function #41721

caesarxuchao opened this issue Sep 30, 2020 · 6 comments
Labels
Milestone

Comments

@caesarxuchao
Copy link

@caesarxuchao caesarxuchao commented Sep 30, 2020

http2.ConfigureTranport configures a net/http HTTP/1 Transport to use HTTP/2, but it does not allow setting http2 specific configurations, e.g., ReadIdleTimeout and PingTimeout. See end-user issue #40201.

The proposal is that adding a ConfigureTransportWithOptions function, which takes a TransportOptions as an input to configure http2 specific configurations.

golang/net#74 is an implementation of this proposal.

@gopherbot gopherbot added this to the Proposal milestone Sep 30, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Oct 14, 2020
@neild
Copy link
Contributor

@neild neild commented Oct 20, 2020

I don't think we need an entirely new representation of HTTP/2 transport configuration; the *http2.Transport type already exposes all the necessary configuration fields. The problem is that http2.ConfigureTransport doesn't provide any way to get at the *http2.Transport it creates.

I propose instead:

package http2

// ConfigureTransports configures a net/http HTTP/1 Transport to use HTTP/2.
// It returns a new HTTP/2 Transport for further configuration.
// It returns an error if t1 has already been HTTP/2-enabled.
func ConfigureTransports(t1 *http.Transport) (*Transport, error) { ... }

Having ConfigureTransport and ConfigureTransports functions with almost identical names is unfortunate, but the two will sort next to each other in godoc and the difference between the two is obvious from the function signature.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 20, 2020

Change https://golang.org/cl/264017 mentions this issue: x/net/http2: add ConfigureTransports

@caesarxuchao
Copy link
Author

@caesarxuchao caesarxuchao commented Oct 21, 2020

That's a good solution! Thanks, @neild.

At first I was concerned that with this solution, the user won't be able to configure the http2 transport (t2) that's registered as the upgrade handler of t1, but then I realized that t2 is a pointer, so the user can just modify the t2 that's returned by ConfigureTransports.

gopherbot pushed a commit to golang/net that referenced this issue Oct 22, 2020
The ConfigureTransport function doesn't provide any way to get at the
http2 Transport it creates, making it impossible to configure transport
parameters such as ReadIdleTimeout.

Add a ConfigureTransports function which returns the http2 Transport.

The very similar names are unfortunate, but they'll sort next to each
other in godoc and the pluralized ConfigureTransports hints at its purpose:
it lets you configure both the http and http2 transports.

Fixes golang/go#40201.
Updates golang/go#41721.

Change-Id: I97aa345f369f49462c41d3f60d35660c06c51287
Reviewed-on: https://go-review.googlesource.com/c/net/+/264017
Trust: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@caesarxuchao
Copy link
Author

@caesarxuchao caesarxuchao commented Oct 28, 2020

The underlying issue was fixed by https://go-review.googlesource.com/c/net/+/264017/.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 31, 2020

Change https://golang.org/cl/266898 mentions this issue: all: update vendored dependencies for Go 1.16 release

@kevinburke kevinburke moved this from Incoming to Likely Accept in Proposals Nov 2, 2020
gopherbot pushed a commit that referenced this issue Nov 11, 2020
The Go 1.16 code freeze has recently started. This is a time to update
all golang.org/x/... module versions that contribute packages to the
std and cmd modules in the standard library to latest master versions.

Those versions have already gone through code review, and now they
will undergo additional testing during the upcoming freeze period.
If new issues in these dependencies are discovered, we have the freeze
period to address them. By the end of the freeze period, we will have
confidence that the Go 1.16 release and the dependency versions it has
selected are robust.

The dependency module versions that are selected in this commit are:

	github.com/google/pprof v0.0.0-20201007051231-1066cbb265c7
	github.com/ianlancetaylor/demangle v0.0.0-20200414190113-039b1ae3a340
	golang.org/x/arch v0.0.0-20201008161808-52c3e6f60cff
	golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897
	golang.org/x/mod v0.3.1-0.20200828183125-ce943fd02449
	golang.org/x/net v0.0.0-20201029221708-28c70e62bb1d
	golang.org/x/sys v0.0.0-20201110211018-35f3e6cf4a65
	golang.org/x/text v0.3.4
	golang.org/x/tools v0.0.0-20201110201400-7099162a900a
	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1

This change was created with a program from CL 256357 patch set 3
(which updates golang.org/x modules only) and the latest bundle tool,
but replacing golang.org/x/net version with a slightly older commit
golang/net@28c70e6 due to #42498:

	$ updatestd -goroot=$HOME/gotip -branch=master
	> go version
	go version devel +ecc3f5112e Thu Nov 5 23:21:33 2020 +0000 darwin/amd64
	> go env GOROOT
	/Users/dmitshur/gotip
	> go version -m /Users/dmitshur/go/bin/bundle
	/Users/dmitshur/go/bin/bundle: go1.15.4
		path	golang.org/x/tools/cmd/bundle
		mod	golang.org/x/tools	v0.0.0-20201110201400-7099162a900a	h1:5E6TPwSBG74zT8xSrVc8W59K4ch4NFobVTnh2BYzHyU=
		dep	golang.org/x/mod	v0.3.0	h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
		dep	golang.org/x/xerrors	v0.0.0-20200804184101-5ec99f83aff1	h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=

	updating module cmd in /Users/dmitshur/gotip/src/cmd
	skipping github.com/chzyer/logex (out of scope, it's not a golang.org/x dependency)
	skipping github.com/chzyer/readline (out of scope, it's not a golang.org/x dependency)
	skipping github.com/chzyer/test (out of scope, it's not a golang.org/x dependency)
	skipping github.com/google/pprof (out of scope, it's not a golang.org/x dependency)
	skipping github.com/ianlancetaylor/demangle (out of scope, it's not a golang.org/x dependency)
	skipping github.com/yuin/goldmark (out of scope, it's not a golang.org/x dependency)
	skipping rsc.io/pdf (out of scope, it's not a golang.org/x dependency)
	> go mod edit -go=1.16
	> go get -d golang.org/x/arch@52c3e6f60cffa0133a3f9b2fc7f6862504a6cba0 golang.org/x/crypto@9e8e0b390897c84cad53ebe9ed2d1d331a5394d9 golang.org/x/mod@ce943fd02449f621243c9ea6e64098e84752b92b golang.org/x/net@28c70e62bb1d140c3f2579fb7bb5095134d9cb1e golang.org/x/sync@67f06af15bc961c363a7260195bcd53487529a21 golang.org/x/sys@35f3e6cf4a65a85bc280e5fe63faed8ac8b25721 golang.org/x/text@22f1617af38ed4cd65b3b96e02bab267e560155c golang.org/x/tools@7099162a900ae8260c5b97cfaf5f374243dfa742 golang.org/x/xerrors@5ec99f83aff198f5fbd629d6c8d8eb38a04218ca
	> go mod tidy
	> go mod vendor

	updating module std in /Users/dmitshur/gotip/src
	> go mod edit -go=1.16
	> go get -d golang.org/x/crypto@9e8e0b390897c84cad53ebe9ed2d1d331a5394d9 golang.org/x/net@28c70e62bb1d140c3f2579fb7bb5095134d9cb1e golang.org/x/sys@35f3e6cf4a65a85bc280e5fe63faed8ac8b25721 golang.org/x/text@22f1617af38ed4cd65b3b96e02bab267e560155c golang.org/x/tools@7099162a900ae8260c5b97cfaf5f374243dfa742
	> go mod tidy
	> go mod vendor

	updating bundles in /Users/dmitshur/gotip/src
	> go generate -run=bundle std cmd

golang.org/x/net will be updated further later, after #42498 is fixed.

github.com/google/pprof and github.com/ianlancetaylor/demangle
contribute packages but are out of scope for this generated CL.

Also rename http2configureTransport in net/http to follow the internal
rename that happened in CL 264017 to fix the build.

For #36905.
Updates #41721.
Updates #42498.

Change-Id: Ifcd2e76f0406e389b6db88041ca51cd0a2115152
Reviewed-on: https://go-review.googlesource.com/c/go/+/266898
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@rsc rsc removed this from Likely Accept in Proposals Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants