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

Closed
gaorong opened this issue Apr 24, 2019 · 12 comments
Closed

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

gaorong opened this issue Apr 24, 2019 · 12 comments
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@gaorong
Copy link

gaorong commented Apr 24, 2019 •
edited

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 bradfitz added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 24, 2019
@bradfitz
Copy link
Contributor

/cc @fraenkel

@fraenkel
Copy link
Contributor

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

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

@gaorong gaorong mentioned this issue Apr 29, 2019
Closed
@szuecs
Copy link

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

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

szuecs commented May 4, 2019

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

@bradfitz
Copy link
Contributor

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

caesarxuchao commented Sep 18, 2019 •
edited

I'll work on a patch.

[Update] WIP

@ereslibre ereslibre mentioned this issue Sep 23, 2019
Closed
caesarxuchao pushed a commit to caesarxuchao/net that referenced this issue Sep 30, 2019
http2: connection pool periodically sends ping frame and closes the
bc0d6c6 
connection if the ping is not responded on time.

DO NOT SUBMIT

Updates golang/go#31643
@caesarxuchao caesarxuchao mentioned this issue Oct 1, 2019
Closed
@gopherbot
Copy link

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

@jqmichael jqmichael mentioned this issue Oct 17, 2019
Closed
@sivaalli sivaalli mentioned this issue Jan 30, 2020
Merged
caesarxuchao pushed a commit to caesarxuchao/net that referenced this issue Feb 22, 2020
net/http2: perform connection health check
60bc432 
After the connection has been idle for a while, periodic pings are sent
over the connection to check its health. Unhealthy connection is closed
and removed from the connection pool.

Updates golang/go#31643
caesarxuchao pushed a commit to caesarxuchao/net that referenced this issue Feb 22, 2020
net/http2: perform connection health check
c5e6610 
After the connection has been idle for a while, periodic pings are sent
over the connection to check its health. Unhealthy connection is closed
and removed from the connection pool.

Fixes golang/go#31643
@fraenkel fraenkel mentioned this issue Feb 22, 2020
Open
caesarxuchao pushed a commit to caesarxuchao/net that referenced this issue Mar 11, 2020
net/http2: perform connection health check
bce9e97 
After the connection has been idle for a while, periodic pings are sent
over the connection to check its health. Unhealthy connection is closed
and removed from the connection pool.

Fixes golang/go#31643
caesarxuchao pushed a commit to caesarxuchao/net that referenced this issue Mar 11, 2020
net/http2: perform connection health check
36607fe 
After the connection has been idle for a while, periodic pings are sent
over the connection to check its health. Unhealthy connection is closed
and removed from the connection pool.

Fixes golang/go#31643
@AceHack AceHack mentioned this issue Apr 9, 2020
Closed
@f4nff f4nff mentioned this issue Jun 10, 2020
Closed
@ostcar ostcar mentioned this issue Jun 18, 2020
Closed
@gopherbot
Copy link

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

@riking riking mentioned this issue Jun 26, 2020
Open
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Aug 25, 2020
@odeke-em
src/go.mod, net/http: update bundled and latest golang.org/x/net
3d77461 
Updates x/net/http2 to git rev c89045814202410a2d67ec20ecf177ec77ceae7f

    http2: perform connection health check
    https://golang.org/cl/198040 (fixes #31643)

    http2: use ASCII space trimming for parsing Trailer header
    https://golang.org/cl/231437

    all: update golang.org/x/crypto to v0.0.0-20200622213623-75b288015ac9
    https://golang.org/cl/239700 (updates #30965)

    net/http2: fix erringRoundTripper
    https://golang.org/cl/243257 (updates #40213)

also updates the vendored version of golang.org/x/net as per

$ go get golang.org/x/net@c890458142
$ go mod tidy
$ go mod vendor
$ go generate -run bundle std

Change-Id: Iea2473ef086df760144d9656f03a0218eb9da91f
Reviewed-on: https://go-review.googlesource.com/c/go/+/249842
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

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

@morvencao morvencao mentioned this issue Jan 18, 2021
Closed
@golang golang locked and limited conversation to collaborators Oct 13, 2021
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
@rsc
http2: perform connection health check
019088f 
After the connection has been idle for a while, periodic pings are sent
over the connection to check its health. Unhealthy connection is closed
and removed from the connection pool.

Fixes golang/go#31643

Change-Id: Idbbc9cb2d3e26c39f84a33e945e482d41cd8583c
GitHub-Last-Rev: 36607fe185ce6684e9900403f82a298ad5567650
GitHub-Pull-Request: golang/net#55
Reviewed-on: https://go-review.googlesource.com/c/net/+/198040
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http2: perform connection health check caesarxuchao/net
6 participants
@bradfitz @szuecs @fraenkel @caesarxuchao @gopherbot @gaorong