Subject: [quicwg/base-drafts] Including ACK delay in packet loss detection time threshold (#3951)

From: Dirkjan Ochtman <[email-masked]>
Date: Thu, 23 Jul 2020 05:32:58 -0700

In https://github.com/djc/quinn/issues/815, I've been going through the handling of PTO and other timer-related code in the Quinn implementation to make sure that it is consistent and compliant with the spec. One issue I bumped on is whether/when the ACK delay is relevant to a particular timer. This seemed somewhat inconsistent to me in the code, so I dug through the recovery spec and found the following:

* [Time Treshold](https://quicwg.org/base-drafts/draft-ietf-quic-recovery.html#time-threshold) for detecting packet loss says the time threshold is `max(kTimeThreshold * max(smoothed_rtt, latest_rtt), kGranularity)`, so it does not include the ACK delay. This seems surprising to me, as the ACK delay seems highly relevant in this case...

* [Idle Timeout](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-idle-timeout) says "To avoid excessively small idle timeout periods, endpoints MUST increase the idle timeout period to be at least three times the current Probe Timeout (PTO)."; it seems like the ACK delay is relevant here: we wouldn't want to consider a connection idle if we haven't received ACKs because they are delayed.

* [Failed Path Validation](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-failed-path-validation) says `validation_timeout = max(3*PTO, 6*kInitialRtt)`. However, path validation should be based on a `PATH_RESPONSE` frame, not an `ACK` frame, so including the `max_ack_delay` here doesn't seem relevant from first principles.

* [Closing And Draining Connection States](https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-closing-and-draining-connec) says "These states SHOULD persist for at least three times the current Probe Timeout (PTO) interval as defined in [QUIC-RECOVERY].", so I think this should keep the ACK delay as well.

So concrete questions I came up with:

* Should the packet loss detection threshold take the `max_ack_delay` into account somehow?
* Should the failed path validation timeout use a different calculation that doesn't include `max_ack_delay`?
* For closing/draining connection timers, does it make sense to include the `max_ack_delay`?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951

---

From: Martin Thomson <[email-masked]>
Date: Thu, 23 Jul 2020 08:05:06 -0700

I am comfortable with this discrepancy (or all three).

The loss detection threshold is based on observing a gap, so it only applies when there is an obligation to acknowledge immediately if the ACK is received.  Thus the max_ack_delay doesn't apply.

The failed path validation timeout simply uses a value that allows for some loss and a response. Any value suffices here, but this value ensures that there is ample opportunity to get a response back.

The closing and draining states follow similar logic to path validation.  Any value would suffice, and in this case the goal is only to avoid silly values being set and connections timing out before you give PTO a chance to repair losses.

(I just successfully simulated a handshake with a 25s RTT, 10% loss, and a 30s idle timeout.  This last safeguard is relevant there as long as you avoid an idle timeout that is less than the RTT.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951#issuecomment-663059678

---

From: Benjamin Saunders <[email-masked]>
Date: Thu, 23 Jul 2020 10:25:55 -0700

While in a typical case the inclusion/omission does seem insignificant, `max_ack_delay` can take values over 16 *seconds*, leading to a quite noticable impact in behavior. If that's nonetheless deemed within reasonable bounds, I think it'd be helpful to have an explicit statement of intent in the text stating that the inclusion of `max_ack_delay` in non-ACK-related timers is for simplicity.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951#issuecomment-663132990

---

From: ianswett <[email-masked]>
Date: Fri, 24 Jul 2020 10:12:42 -0700

max_ack_delay isn't included in time threshold loss detection because the transport document says you need to acknowledge packets which were previously missing immediately and that out of order packets are acknowledged immediately.  So not including it is because it's not necessary to include it, and including it would slow down loss detection, in some cases substantially.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951#issuecomment-663641541

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 24 Jul 2020 15:37:27 -0700

Is there anything to be done here?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951#issuecomment-663760777

---

From: Benjamin Saunders <[email-masked]>
Date: Sun, 26 Jul 2020 10:58:06 -0700

Some clarifying language expressing that the ack delay is included in path validation/closing/draining timers/etc only for simplicity would be nice. Given the large potential impact, the reader is otherwise left trying to guess why that's appropriate.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951#issuecomment-664020514

---

From: ianswett <[email-masked]>
Date: Sun, 26 Jul 2020 12:47:09 -0700

Thanks, that sounds editorial.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/issues/3951#issuecomment-664032034