HTTP/2: throttle validateAfterInactivity pre-flight PING checks

[arturobernalg]

Throttle the validateAfterInactivity pre-flight PING logic with a fixed granularity.
Prevents excessive PING traffic when validateAfterInactivity is set to tiny values (e.g. 1ms).

[ok2c]

@arturobernalg The change-set will likely need some work, but I am conceptually fine with it. @rschmitt Would you have any conceptual objections to the idea?

[rschmitt]

When enabled, the client sends PING frames once the SETTINGS handshake is complete and no streams are active, and fails the session if the peer does not respond with PING[ACK] within the configured timeout.

This sounds way better than the current HTTP/2 implementation of setValidateAfterInactivity, which adds an entire round trip's worth of latency to the request path. Would it be outrageous if exposed this through setValidateAfterInactivity instead of adding something like a new H2Config option?

[ok2c]

@arturobernalg

1. I also think H2Config is not the right place. Keep it as a separate constructor parameter. Once available in client H2PingPolicy should go into ConnectionConfig
2. Any chance you could use the existing ping command here?

[rschmitt]

Once available in client H2PingPolicy should go into ConnectionConfig

I think we have too many config knobs already. I'd really prefer to expose this through setValidateAfterInactivity. Once we have support for HTTP/2 keep-alive PINGs that run asynchronously in the background, no one in their right mind will ever want to send a blocking PING in the request path to validate a connection. (I've spent this week dealing with a set of production issues that are directly related to this problem, and it has radicalized me re: how badly I want this keep-alive PING functionality.)

[arturobernalg]

keep-alive is now driven via setValidateAfterInactivity (no new H2Config knob), so it fits the existing connection validation API.
Also switched to the existing ping command path (keep-alive uses the same sendPing flow), per your suggestion.

[arturobernalg]

keep-alive is now driven via setValidateAfterInactivity (no new H2Config knob), so it fits the existing connection validation API. Also switched to the existing ping command path (keep-alive uses the same sendPing flow), per your suggestion.

@ok2c should i do another change here?

[ok2c]

@arturobernalg I like the fact that API changes are small, but I find the implementation too complex and overwrought:

The process should be quite simple:

• on timeout shut down immediately if the connection has not been fully initialized, SETTINGS handshake has not completed or the keep-alive has not been turned on
• otherwise set timeout to something like 1 second and send ping
• if ping comes back restore the original timeout
• if a timeout event fires again, shut down the connection and go home

[arturobernalg]

I like the fact that API changes are small, but I find the implementation too complex and overwrought:

The process should be quite simple:

* on timeout shut down immediately if the connection has not been fully initialized, SETTINGS handshake has not completed or the keep-alive has not been turned on

* otherwise set timeout to something like 1 second and send ping

* if ping comes back restore the original timeout

* if a timeout event fires again, shut down the connection and go home

@ok2c I reworked the keep-alive handling to follow the exact 4-step process you outlined

[ok2c]

@arturobernalg Very good. Almost there. One last thing we should do is to make sure does not fire more frequently than 1 or 3 seconds even if one sets validateAfterInactivity to 1 ms or something equally crazy.