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

[Marten Seemann <[e-mail]>]

marten-seemann commented on this pull request.

It's a big annoying that we need the `GetEarliestLossTime()` function for something that trivial.
If we initialize the loss times to infinity, taking the minimum would become more straightforward, but we'd still need some code to tell which packet number space the chosen time comes from, so I'm not sure if this change would really help.

> @@ -1022,13 +1021,31 @@ timers wake up late. Timers set in the past SHOULD fire immediately.
 Pseudocode for SetLossDetectionTimer follows:
 
 ~~~
+// Returns the earliest loss_time and the packet number
+// space it's from.  Returns 0 if all times are 0.
+GetEarliestLossTime():
+  time = loss_time[Initial]
+  space = Initial
+  for pn_space in [ Handshake, ApplicatonData ]:
+    if loss_time[pn_space] != 0 &&
+       (time == 0 || loss_time[pn_space] < time):
+      time = loss_time[pn_space];
+      space = Initial

`space = pn_space`

>  SetLossDetectionTimer():
   // Don't arm timer if there are no ack-eliciting packets
   // in flight.
   if (no ack-eliciting packets in flight):
     loss_detection_timer.cancel()
     return
 
+  [loss_time, pn_space] = GetEarliestLossTime()

In Python, you don't need square brackets to receive multiple return values.

And since we're not using the the `pn_space` here, we could write: `loss_time, _ = GetEarliestLossTime()`

> @@ -1067,14 +1078,15 @@ Pseudocode for OnLossDetectionTimeout follows:
 
 ~~~
 OnLossDetectionTimeout():
-  if (crypto packets are in flight):
+  [loss_time, pn_space] = GetEarliestLossTime()

Same here.

> @@ -1067,14 +1078,15 @@ Pseudocode for OnLossDetectionTimeout follows:
 
 ~~~
 OnLossDetectionTimeout():
-  if (crypto packets are in flight):
+  [loss_time, pn_space] = GetEarliestLossTime()
+  if (loss_time != 0):
+    // Time threshold loss Detection
+    DetectLostPackets(pn_space)
+  // Don't retransmit all crypto data if a packet was just lost.

This comment only makes sense for someone who followed the evolution of this algorithm. We should instead have a comment that describes when to retransmit all crypto data.

-- 
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-202436241