quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

`keep_alive_interval` can be larger than `max_idle_timeout` #1670

Open kixelated opened 1 year ago

kixelated commented 1 year ago

TransportConfig contains two settings:

It seems natural that you just set Y < X and the connection will never get closed. However, max_idle_timeout is a negotiated parameter, so the peer could choose Z where Z < X. If Z < Y then the connection will be closed before the keep alive PING is sent.

I think there's two options to fix this:

  1. Expose max_idle_timeout and set_keep_alive_interval on Connecting and/or Connection.
  2. Switch keep_alive to a bool, automatically setting the interval based on max_idle_timeout.

I actually prefer the 2nd option because it's transparent and can be smart.

Currently, max_idle_timeout = 10s and keep_alive_interval = 6s might not survive from a single packet loss.

The best behavior would be to send staggered PING packets close to the max_idle_timeout. The keep_alive timer would reset on packet sent, much like the idle_timer is reset on packet received.

Also from my testing, Chrome closes the connection a second earlier than max_idle_timeout. It sounds like a bug; they probably meant to close a second after the idle timeout. This smart keep-alive logic should unfortunately take that into account or be configurable.

Ralith commented 1 year ago

I agree that a simple setting that Just Works is desirable, but I'm not confident that we can select a tradeoff between likelihood of loss recovery and number of empty packets which suits all applications, which might span from hardwired LAN gaming to deep space networking.

I'm also not sure if it's ever necessary to dynamically inspect the peer's idle timeout, since it's not clear why this might vary unpredictably for both sides. In what case can neither endpoint have reasonable advance knowledge of the other's idle timeout?