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: make Transport occasionally send PING frames to server #31643

Open
gaorong opened this issue Apr 24, 2019 · 8 comments

Comments

@gaorong
Copy link

commented Apr 24, 2019

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?

@gaorong gaorong changed the title [feature request] support all connection in default `clientConnPool` periodically send ping fame to peer [feature request] support all connection in default `clientConnPool` periodically send ping frame to peer Apr 24, 2019

@bradfitz bradfitz changed the title [feature request] support all connection in default `clientConnPool` periodically send ping frame to peer x/net/http2: make Transport occasionally send PING frames to server Apr 24, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

/cc @fraenkel

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@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

This comment has been minimized.

Copy link

commented Apr 27, 2019

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

@szuecs

This comment has been minimized.

Copy link

commented May 4, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@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

This comment has been minimized.

Copy link

commented May 4, 2019

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

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

This comment has been minimized.

Copy link

commented Sep 18, 2019

I'll work on a patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.