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: no public method to send PING frames #15475

Closed
blixt opened this issue Apr 28, 2016 · 7 comments
Closed

x/net/http2: no public method to send PING frames #15475

blixt opened this issue Apr 28, 2016 · 7 comments

Comments

@blixt
Copy link
Contributor

blixt commented Apr 28, 2016

There is, as far as I can tell, no method for sending a PING frame to an open HTTP/2 connection right now. While in most cases there is no direct need to send PING frames, I've encountered multiple cases where it's important, lest you discard the connection and recreate it:

  • Apple's APNs HTTP/2 API is picky about what it receives and only supports PING frames as a way to ensure a healthy connection. Sending dummy requests may get your connection closed.
  • Go's HTTP/2 lib chokes on idle sockets on Google Compute Engine VMs (see x/net/http2: transport.CloseIdleConnections() does not have any effect #14607 for some info). If the socket happens to be an HTTP/2 connection (which it often is in my case), being able to send PING frames would be a viable workaround for this issue.
  • Amazon's AVS HTTP/2 API expects a PING frame at least once per 10 minutes and it should be possible to send them. (Luckily, there's a workaround to call /ping in this case.)

So my suggestion is to somehow expose a way to send PING frames.

@bradfitz bradfitz self-assigned this Apr 28, 2016
@bradfitz bradfitz added this to the Unreleased milestone Apr 28, 2016
@zjx20
Copy link

zjx20 commented May 5, 2016

I suggest that x/net/http2 should periodically send PING frames to the peer, and kick muted connections from the connection pool automatically. This feature should be off by default, and could be turned on by setting some kind of options.

@szpakas
Copy link

szpakas commented May 9, 2016

Like @blixt, I'm implementing APNS client and lack of access to PING frame is a bit of a show stopper for pushing to prod environment. It makes sense to have periodical ping as proposed by @zjx20, but it would be great to have also some kind of read-only access to connection stats for both PING and GOAWAY frames (to have connection behaviour in metrics).

@bradfitz: As I'm on a bit tight schedule and not sure can wait for the official fix, could You recommend some temporary solution which will allow access to ping on client side? I'm still quite green in GO and can't figure out how to extend the connection without actually forking x/net/http2 and making exported getter for Framer in http2ClientConn.

@tombergan
Copy link
Contributor

PING frames are also useful for measuring the true end-to-end RTT, which you can't reliably measure at the transport level due to split TCP in cellular networks. I don't have a need for this feature, but if it's implemented, I would vote for a blocking API so the caller can time the RTT. Something like:

func (s *h2connection) Ping(opaque uint64) (uint64, error)

If you need to PING periodically, you could always do so from a timer loop.

@bradfitz
Copy link
Contributor

@tombergan, what is the return uint16 in your example? If it's the data, it must match the ping opaque value in the sent frame, so it's redundant, no?

@tombergan
Copy link
Contributor

tombergan commented Jun 23, 2016

That's right, I missed that the ACK must echo the opaque value from the original PING. The uint64 return value is not necessary. Further, the connection should probably manage opaque values so it can match pings with their ACKs:

func (s *h2connection) Ping() error

@rs
Copy link
Contributor

rs commented Sep 29, 2016

Any update on this?

I think this ping method should get a context as first argument to let the caller manage deadline and cancellation. Something like:

func (cc *ClientConn) Ping(ctx context.Context) error

@rs
Copy link
Contributor

rs commented Sep 29, 2016

Here is an implementation proposal: https://go-review.googlesource.com/#/c/29965/

@golang golang locked and limited conversation to collaborators Sep 29, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
This new method sends a PING frame with random payload to the peer and
wait for a PING ack with the same payload.

In order to support cancellation and deadling, the Ping method takes a
context as argument.

Fixes golang/go#15475

Change-Id: I340133a67717af89556837cc531a885d116eba59
Reviewed-on: https://go-review.googlesource.com/29965
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants