x/net/http2: missing httptrace.GetConn hook

[unkaktus]

http2 implementation does not call GetConn hook before getting connection from pool [1]. As per net/http/httptrace documentation:

GetConn is called even if there's already an idle cached connection available.

For the record, this breaks clients that rely on GetConn->GotConn timings (such as hey benchmark) so they report wrong results.

[1] https://github.com/golang/net/blob/master/http2/transport.go#L353

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.9.2 openbsd/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="openbsd"
GOOS="openbsd"
GOPATH="/home/gopher/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/openbsd_amd64"
GCCGO="gccgo"
CC="cc"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build311252074=/tmp/go-build -gno-record-gcc-switches"
CXX="c++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

package main

import (
        "context"
        "log"
        "net/http"
        "net/http/httptrace"
)

func main() {
        log.SetFlags(0)
        t := http.DefaultTransport
        for i := 0; i < 3; i++ {
                trace := &httptrace.ClientTrace{
                        GetConn: func(hostport string) {
                                log.Printf("GetConn")
                        },
                        GotConn: func(gci httptrace.GotConnInfo) {
                                log.Printf("GotConn")
                        },
                }
                ctx := httptrace.WithClientTrace(context.Background(), trace)
                r, _ := http.NewRequest("GET", "https://golang.org/", nil)
                _, err := t.RoundTrip(r.WithContext(ctx))
                if err != nil {
                        log.Fatal(err)
                }
        }
}

What did you expect to see?

GetConn
GotConn
GetConn
GotConn
GetConn
GotConn

What did you see instead?

GetConn
GotConn
GotConn
GotConn

[bradfitz]

/cc @tombergan

[bradfitz]

By default this would be for Go 1.11 if this is a bug (I assume it is), but if it's a trivial fix I wouldn't fight it going into Go 1.10 either.

[unkaktus]

I guess it's a trivial fix because traceGetConn is simply not implemented.
I've fixed it just by implementing traceGetConn akin traceGotConn and plugging in before connPool().GetClientConn().

[unkaktus]

Should I create a CL?

[bradfitz]

Go for it.

[odeke-em]

Gentle ping @nogoegst, as Go1.11 is coming up in less than a month.

[gopherbot]

Change https://golang.org/cl/122590 mentions this issue: http2: call httptrace.ClientTrace.GetConn in Transport when needed

[gopherbot]

Change https://golang.org/cl/122591 mentions this issue: net/http: update bundled http2