Go: x/net/http2: make Transport occasionally send PING frames to server

[gaorong]

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

$ go version
go version go1.12.4 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output

$ go env

GOARCH="amd64"

GOBIN=""

GOCACHE="/home/gaorong1/.cache/go-build"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GOOS="linux"

GOPATH="/home/gaorong1"

GOPROXY=""

GORACE=""

GOROOT="/usr/local/go"

GOTMPDIR=""

GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"

GCCGO="gccgo"

CC="gcc"

CXX="g++"

CGO_ENABLED="1"

GOMOD=""

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build866234888=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Nowadays, the ClientConn in x/net/http2 package has supported to send ping frame to peer, but the related default clientConnPool doesn't support this feature and clientConnPool is an internal struct, we couldn't reuse this struct and add our own logic, As a result, If we want all connections in a connection pool to send ping fame to keep connection alive and identify failed connections, we must implement a new customized connpool, which seems a hard work to do.
IMHO, maybe we should add support in default clientConnPool to let all connection periodically send ping fame to peer?

What did you expect to see?

add support in default clientConnPool to let all connection periodically send ping fame to peer?

What did you see instead?

[bradfitz]

/cc @fraenkel

[fraenkel]

@bradfitz One idea would be to have a ConfigureWrappedTransport(t1 *http.Transport, func(ClientConnPool) ClientConnPool) error. We can wrap the underlying connection pool with whatever is provided and let the user decide on when/how often to invoke Pings based on the lifetime of a ClientConn.

If we want this baked into the clientConnPool, we currently have no good way to pass additional information forward, e.g. ping interval.

While this is a client->server issue, does anyone ever do server -> client initiated?

[gopherbot]

Change https://golang.org/cl/173952 mentions this issue: http2: allow a means to obtain the client connection

[szuecs]

@fraenkel in what situations server->client would be used?
I think a client starts a connection, therefore sends Ping frames to check if server is gone on long poll connections. Server wants to do tear down, which has to send Goaway. Client should accept Goaway and close connection.
All this is true for http proxies, too.

I am not sure about if it would be useful for server push connections.

[fraenkel]

@szuecs This is partially about supporting the http/2 spec which allows either endpoint to initiate a ping. A server may want to know if a client is no longer responsive. Today a server relies on the connection being severed but there are many cases where a connection remains active but the client is no longer present, e.g., proxies, or unresponsive. A PING provides an alternative way.

[szuecs]

@fraenkel so instead of CloseIdleConnections a proxy could use PING, instead. Thanks

[bradfitz]

We should probably send occasional PINGs by default on idle connections (configurable, including off), but to get fancier we could also we could send a PING along with any HEADERS if it's been more than some certain threshold duration since the last PING. We can then retry the request on a new connection if we don't get a PING response back soon. We can only do that safely with idempotent+replayable (bodyless or Request.GetBody != nil) so for others (e.g. POST without GetBody) we could do the PING first over a certain threshold and just pay the extra RTT cost.

But probably easiest to start with just occasional PINGs.

[caesarxuchao]

I'll work on a patch.

[Update] WIP

[gopherbot]

Change https://golang.org/cl/198040 mentions this issue: http2: connection pool periodically sends ping frame and closes the

[gopherbot]

Change https://golang.org/cl/238721 mentions this issue: [release-branch.go1.13] http2: perform connection health check

[gopherbot]

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

[gopherbot]

Change https://golang.org/cl/258538 mentions this issue: [release-branch.go1.14] src, net/http: update vendor, regenerate h2_bundle.go