Consider to send PING as a keeplive

[KershawChang]

See bug 1914309. I assume it's that the server took more than 30 seconds to respond, leading to the connection being closed due to idle timeout.

Sending a PING as a keepalive when there are outstanding requests could help fix this issue. Additionally, Chrome has already implemented this behavior, so it might be worth considering for us as well.

[martinthomson]

My understanding is that we already have the capability in the stack.

When we create a connection, we set the keepalive flag on that stream.

The bug indicates that something isn't working. But I can't see how we wouldn't be sending a PING as needed. Investigation might be needed.

[larseggert]

Do we have a test for this situation? If yes, is it correct? If no, let's add one and see what the result is.

[martinthomson]

There are plenty of tests from here on down:

neqo/neqo-transport/src/connection/tests/idle.rs

Line 408 in 28f60bd

fn keep_alive_initiator() {

[KershawChang]

Got another bug 1920697 about this.

2024-09-25 10:52:42.196762 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessInput writer=1315c0c40 [this=14fde4100 state=2]
2024-09-25 10:52:42.196813 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessEvents [this=14fde4100]
2024-09-25 10:52:42.196817 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::SendData [this=14fde4100]
2024-09-25 10:52:42.196820 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessOutput reader=1315c0c40, [this=14fde4100]
2024-09-25 10:52:42.196829 UTC - [Parent 52801: Socket Thread]: I/nsHttp Http3Session::SetupTimer to 29999ms [this=14fde4100].
...
...
2024-09-25 10:53:12.262987 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::SendData [this=14fde4100]
2024-09-25 10:53:12.262993 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessOutput reader=1315c0c40, [this=14fde4100]
2024-09-25 10:53:12.263107 UTC - [Parent 52801: Socket Thread]: I/neqo_transport::* [neqo_transport::connection] [Client 71e3d6db510060fae2c514b77d] idle timeout expired
2024-09-25 10:53:12.263208 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessEvents [this=14fde4100]
2024-09-25 10:53:12.263224 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessEvents - ConnectionClosed
2024-09-25 10:53:12.263286 UTC - [Parent 52801: Socket Thread]: V/nsHttp Http3Session::ProcessEvents - ConnectionClosed error=804b000e

The log shows that the connection was closed due to idle timeout and we don't send any ping within 30s.

I'll try to write a xpcshell test and see if I can create a neqo test case to reproduce.

[KershawChang]

So, I think this is really regressed by #1491.

We do have a test for this case.

neqo/neqo-transport/src/connection/tests/idle.rs

Lines 493 to 498 in 55e3a93

	// Wait that long and the client should send a PING frame.
	now += default_timeout() / 2;
	let pings_before = client.stats().frame_tx.ping;
	let ping = client.process_output(now).dgram();
	assert!(ping.is_some());
	assert_eq!(client.stats().frame_tx.ping, pings_before + 1);

The test waits for default_timeout() / 2 and then calls process_output, which successfully sends a PING.
However, the problem is at the result of the previous process_output call.

neqo/neqo-transport/src/connection/tests/idle.rs

Line 491 in 55e3a93

assert_idle(&mut client, now, default_timeout());

It tells the caller to wait the whole idle timeout. As a result, if process_output isn't called again within idle timeout, the PING is never sent.

I am going to write a PR to fix this.