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: ConfigureTransport does not set h2transport on the http.Transport #22891

Closed
nhooyr opened this issue Nov 27, 2017 · 4 comments
Closed
Assignees
Milestone

Comments

@nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Nov 27, 2017

If the TLSClientConfig field of http.Transport is not set, then the HTTP package creates a default one with the following function that enables HTTP2:

t2, err := http2configureTransport(t)
if err != nil {
log.Printf("Error enabling Transport HTTP/2 support: %v", err)
return
}
t.h2transport = t2

However, if it is set and HTTP2 is enabled, then because of the following lines:

if t.TLSClientConfig != nil || t.Dial != nil || t.DialTLS != nil {
// Be conservative and don't automatically enable
// http2 if they've specified a custom TLS config or
// custom dialers. Let them opt-in themselves via
// http2.ConfigureTransport so we don't surprise them
// by modifying their tls.Config. Issue 14275.
return
}

The h2transport field of the http.Transport will never be set and so the CloseIdleConnections method of http.Transport will never close http2 connections.

See

if t2 := t.h2transport; t2 != nil {

What this means is that setting a custom TLS client config and not setting one does not allow the same range of functionality. I think this is a bug because using a custom HTTP2 enabled TLSClientConfig should be just as capable as not setting one.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 29, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Apr 24, 2018

/cc @rs too

@bradfitz bradfitz self-assigned this Jul 12, 2018
@bradfitz bradfitz added the NeedsFix label Jul 12, 2018
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 12, 2018

Change https://golang.org/cl/123656 mentions this issue: http2: export a field of an internal type for use by net/http

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 12, 2018

Change https://golang.org/cl/123657 mentions this issue: net/http: make Transport.CloseIdleConnections close non-bundled http2.Transport

gopherbot pushed a commit to golang/net that referenced this issue Jul 12, 2018
Updates golang/go#22891

Change-Id: Ibde5ce0867a78703a5a4f04fafc3d709ea4cbda3
Reviewed-on: https://go-review.googlesource.com/123656
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot gopherbot closed this in f22dd66 Jul 12, 2018
@golang golang locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.