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

x/net/http2: ConfigureTransport does not set h2transport on the http.Transport #22891

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

x/net/http2: ConfigureTransport does not set h2transport on the http.Transport #22891

nhooyr opened this issue Nov 27, 2017 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@nhooyr
Copy link
Contributor

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:

go/src/net/http/transport.go

Lines 227 to 232 in 1c69384

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:

go/src/net/http/transport.go

Lines 219 to 226 in 1c69384

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

go/src/net/http/transport.go

Line 538 in 1c69384

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

CC @bradfitz @tombergan

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@odeke-em
Copy link
Member

/cc @rs too

@bradfitz bradfitz self-assigned this Jul 12, 2018
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 12, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 12, 2018
@gopherbot
Copy link

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

@gopherbot
Copy link

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
@bradfitz
http2: export a field of an internal type for use by net/http
a1d6821 
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>
@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
5 participants
@bradfitz @ianlancetaylor @odeke-em @gopherbot @nhooyr