net/http/httptest: Server.Close mutates http.DefaultTransport

[tkashem]

Go version

go version go1.21.6 linux/amd64

Output of go env in your module/workspace:

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/akashem/.cache/go-build'
GOENV='/home/akashem/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/akashem/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/akashem/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/akashem/go/src/k8s.io/kubernetes/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2922644753=/tmp/go-build -gno-record-gcc-switches'

What did you do?

It seems that the default transport object http.DefaultTransport gets mutated by the Close method of httptest.Server.

Here is a test that demonstrates it:

func TestHTTPTestServerCloseMutatesDefaultTransport(t *testing.T) {
	server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("pong"))
	}))
	server.EnableHTTP2 = true
	server.StartTLS()

	client := server.Client()
	client.Get(server.URL + "/ping")

	want := http.DefaultTransport.(*http.Transport).TLSClientConfig
	if want != nil {
		t.Errorf("expected TLSClientConfig of http.DefaultTransport to be nil")
	}
	server.Close()
	if got := http.DefaultTransport.(*http.Transport).TLSClientConfig; want != got {
		t.Errorf("TLSClientConfig has been mutated")
	}
}

The TLSClientConfig field of the DefaultTransport object is nil to start with, but then it gets replaced with a non nil instance after the Close method returns. Here is the call chain of the mutation:

httptest/Server.Close invokes the CloseIdleConnections method of the default DefaultTransport object:

go/src/net/http/httptest/server.go

Lines 237 to 242 in cf52e70

	// Not part of httptest.Server's correctness, but assume most
	// users of httptest.Server will be using the standard
	// transport, so help them out and close any idle connections for them.
	if t, ok := http.DefaultTransport.(closeIdleTransport); ok {
	t.CloseIdleConnections()
	}

go/src/net/http/transport.go

Lines 783 to 784 in cf52e70

	func (t *Transport) CloseIdleConnections() {
	t.nextProtoOnce.Do(t.onceSetNextProtoDefaults)

TLSClientConfig.NextProtos gets set:

go/src/net/http/h2_bundle.go

Lines 7269 to 7274 in cf52e70

	if t1.TLSClientConfig == nil {
	t1.TLSClientConfig = new(tls.Config)
	}
	if !http2strSliceContains(t1.TLSClientConfig.NextProtos, "h2") {
	t1.TLSClientConfig.NextProtos = append([]string{"h2"}, t1.TLSClientConfig.NextProtos...)
	}

I am not certain if it's common for production code to exercise:

	if t, ok := http.DefaultTransport.(closeIdleTransport); ok {
		t.CloseIdleConnections()
	}

I ran into this issue with unit tests that verify that the TLSClientConfig field of a given transport does not get mutated unexpectedly. I can change the test to work around it. But I wanted to bring this to your attention so you can assess whether it can impact production code.

[seankhliao]

cc @neild

[seankhliao]

I think the existing situation os fine, since it's an initialization thing, CloseIdleConnections can be called before any use to initialize the fields.