Add HTTP/2 keep-alive PING policy.

[arturobernalg]

Issue a single pre-flight HTTP/2 PING before executing pending commands when idle > validateAfterInactivity.
Use a short ACK timeout and restore the previous socket timeout on completion.

[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 It has to be massively simpler

In AbstractH2StreamMultiplexer at the line 522 do the following

if (hasBeenIdleTooLong && ioSession.hasCommands() && pingHandlers.isEmpty()) {

    ioSession.setSocketTimeout(Timeout.ofSeconds(5));
    executePing(new PingCommand(new BasicPingHandler(new Callback<Boolean>() {

        @Override
        public void execute(final Boolean result) {
            // restore timeout
            ioSession.setSocketTimeout(...);
            if (result) {
                // proceed with command execution
                requestSessionOutput();
            } else {
                ioSession.enqueue(new ShutdownCommand(CloseMode.GRACEFUL), Command.Priority.NORMAL);
            }
        }

    })));
    return;
}

Add idle time tracking, and that should be all it takes.

[arturobernalg]

@ok2c
I reworked it to the massively simpler pre-flight PING: only when idle > validateAfterInactivity and commands are pending, with a short ACK timeout and restoring the previous socket timeout.