Re: [quicwg/base-drafts] Add a loss_time per packet number space (#2451)

[Marten Seemann <[e-mail]>]

marten-seemann commented on this pull request.

This is basically exactly what I just worked out on paper an hour ago, before running into https://github.com/quicwg/base-drafts/issues/2435#issuecomment-462371300.

I believe you forgot to change `OnLossDetectionTimeout()` to the following:
```
OnLossDetectionTimeout():
  min_loss_time = min_non_zero(loss_time[Initial], loss_time[Handshake], loss_time[ApplicationData])
  if (min_loss_time != 0):
    // Time threshold loss Detection
    DetectLostPackets(packet number space with the min_loss_time)
  else if (crypto packets are in flight):
    // Crypto retransmission timeout.
    RetransmitUnackedCryptoData()
    crypto_count++
  else:
    // PTO
    SendOneOrTwoPackets()
    pto_count++

  SetLossDetectionTimer()
```

> @@ -1029,6 +1028,12 @@ SetLossDetectionTimer():
     loss_detection_timer.cancel()
     return
 
+  for (loss_time : loss_times) {

I don't think this guarantees that you're picking the earliest timer. We need something like 
```
min_loss_time = min_non_zero(loss_time[Initial], loss_time[Handshake], loss_time[ApplicationData])
if (min_loss_time != 0):
    loss_detection_timer.update(min_loss_time)
    return
```

> @@ -1117,11 +1115,11 @@ DetectLostPackets(pn_space):
       if (unacked.in_flight):
         lost_packets.insert(unacked)
     else if (pn_space == ApplicationData):

Just `else`, no `else if`.

> @@ -873,10 +873,9 @@ max_ack_delay:
   received ACK frame may be larger due to late timers, reordering,
   or lost ACKs.
 
-loss_time:
-: The time at which the next packet will be considered lost based on
-  exceeding the reordering window in time. Only applies to the ApplicationData
-  packet number space.
+loss_times[kPacketNumberSpace]:

Nit: `loss_time` would be more consistent, since we also use `largest_acked_packet[kPacketNumberSpace]`, not `largest_acked_packets[kPacketNumberSpace]`.

-- 
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/pull/2451#pullrequestreview-202171753