Subject: [quicwg/base-drafts] SetLossDetectionTime negative timeout (#2052)

From: Lars Eggert <[email-masked]>
Date: Mon, 26 Nov 2018 02:15:11 -0800

In `OnPacketSent()`, `time_of_last_sent_retransmittable_packet` is set to `now`, and then `SetLossDetectionTimer()` is called.

In `SetLossDetectionTimer()`, the early RTX timer is computed as `timeout = loss_time - time_of_last_sent_retransmittable_packet`, and `loss_time` can be smaller than `now`, which causes `timeout` to go negative.

-- 
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/2052

---

From: janaiyengar <[email-masked]>
Date: Mon, 26 Nov 2018 20:08:52 -0800

This could use some clarifying, but that shouldn't happen. loss_time is set to 0 if loss_time is smaller than now, see DetectLostPackets() which sets loss_time. Specifically,
```
    if (time_since_sent > delay_until_lost ||
        delta > reordering_threshold):
      sent_packets.remove(unacked.packet_number)
      if (unacked.retransmittable):
        lost_packets.insert(unacked)
    else if (loss_time == 0 && delay_until_lost != infinite):
      loss_time = now() + delay_until_lost - time_since_sent
```
Since loss_time is *not set* when time_since_sent > delay_until_lost, it remains set to 0. This means an ER timer does not need to be set (and is not set in SetLossDetectionTimer(), which is exactly what you want, since setting a timer only makes sense if the event is in the future. In this case, loss_time was before now(), and so, the loss event happens now, causing OnPacketsLost() to be called at the end of DetectLostPackets().

-- 
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/2052#issuecomment-441899285

---

From: Lars Eggert <[email-masked]>
Date: Mon, 26 Nov 2018 22:32:51 -0800

Consider the case that `loss_time` is set in `DetectLostPackets()`, but by the time `SetLossDetectionTimer()` is called, `now` is larger than `loss_time`.

-- 
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/2052#issuecomment-441944567

---

From: ianswett <[email-masked]>
Date: Tue, 27 Nov 2018 15:44:29 +0000 (UTC)

To clarify, this is a case when alarms wake up late, since ideally if this was negative, the alarm should have executed, correct?

I think the easiest fix that fits with how most alarms work is to allow negative values and say they mean execute ASAP.

-- 
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/2052#issuecomment-442106240

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 28 Nov 2018 01:58:59 +0000 (UTC)

For somebody unfamiliar with congestion control, it helps if the pseudo-code in the specification was designed to always return a non-negative timeout, because then, you could check the value to see if you have implemented it correctly.

-- 
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/2052#issuecomment-442289506

---

From: Martin Thomson <[email-masked]>
Date: Tue, 27 Nov 2018 18:21:52 -0800

I think that Kazuho is right.  `timeout = max(timeout, 0)` isn't that hard to add to the algorithm.  An unchecked underflow on an unsigned integer would be an unfortunate outcome.

-- 
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/2052#issuecomment-442293982

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 28 Nov 2018 00:27:22 -0800

@ianswett 
> To clarify, this is a case when alarms wake up late, since ideally if this was negative, the alarm should have executed, correct?

I could well be missing something, but my understanding that the case can happen with accurate alarms.

Consider the case when an endpoints view of packets are as follows:

|PN|sent_at|acked?|notes|
|---|---|---|---|
|1|0|Y|sent normally|
|2|100|N|sent normally|
|3|300|N|sent due to RTO|

At this point, the loss timer is set to 700 (i.e. 2 \* min_rto_timeout (200)).

Then, when the endpoint receives a packet containing an ACK for PN=2 at time 600, `rto_count` is reset to zero, because PN=2 is newly acked. Then, the loss timer becomes 500, which is 100 earlier than the current time.

-- 
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/2052#issuecomment-442360989

---

From: ianswett <[email-masked]>
Date: Wed, 28 Nov 2018 13:37:01 -0800

@martinthomson if the timeout is unsigned, then max(0, timeout) doesn't help because timeout has already wrapped :(

@kazuho Interesting example I've never considered.  However, that doesn't cause an int underflow, just for the timer to be set in the past.

I sent out a PR to avoid the int underflow, because it's easy to fix and I agree it's best to avoid negative values when possible.

I'd rather not do anything to prevent timers being set in the past, because it can happen almost anywhere under some circumstances(ie: late alarms).

-- 
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/2052#issuecomment-442613732