Subject: [quicwg/base-drafts] Add an enum indicating the timer_mode (#3182)

From: ianswett <[email-masked]>
Date: Fri, 01 Nov 2019 10:46:07 -0700

After reading this, it feels a bit duplicative to whether a loss_time is present to me, but I&#39;m writing a PR so people can take a look.

Fixes #3151
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3182

-- Commit Summary --

  * Add an enum indicating the timer_mode

-- File Changes --

    M draft-ietf-quic-recovery.md (19)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3182.patch
https://github.com/quicwg/base-drafts/pull/3182.diff

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

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 01 Nov 2019 14:45:12 -0700

janaiyengar commented on this pull request.

I can see this potentially being easier to read. A few comments.

> @@ -1151,12 +1157,13 @@ Pseudocode for OnLossDetectionTimeout follows:
 
 ~~~
 OnLossDetectionTimeout():
-  loss_time, pn_space = GetEarliestLossTime()
-  if (loss_time != 0):
-    // Time threshold loss Detection
-    DetectLostPackets(pn_space)
-    SetLossDetectionTimer()
-    return
+  if (timer_mode == LOSS_DETECTION):
+    loss_time, pn_space = GetEarliestLossTime()
+    if (loss_time != 0):

So, this should not be required. Specifically, I think what we might want here is an ASSERT(loss_time != 0), though an assert might be too harsh. Maybe a warning and return.

Also below, under the else for a PTO, add an assert that mode is PTO.

> @@ -1151,12 +1157,13 @@ Pseudocode for OnLossDetectionTimeout follows:
 
 ~~~
 OnLossDetectionTimeout():

Perhaps rename this to OnTimeout

> @@ -932,6 +932,10 @@ max_ack_delay:
 loss_detection_timer:

rename this to "timer", with description "multi-modal timer used for loss detection and for sending probes."

-- 
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/3182#pullrequestreview-310708974

---

From: Marten Seemann <[email-masked]>
Date: Fri, 01 Nov 2019 18:36:31 -0700

@ianswett Thanks for the PR! I was planning to submit a PR myself to fix the issue I raised, but I got sidetracked by other stuff.

Don't we also need a timer mode for the amplification limit on the client side?

-- 
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/3182#issuecomment-548998009

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 04:46:37 -0700

I am treating(and think of) the client amplification mode as a case of PTO, but it sounds like you're writing a PR, so let's wait for that.

-- 
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/3182#issuecomment-549036449

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 04:48:12 -0700

ianswett commented on this pull request.

 ~~~

I think we either need an if check or an ASSERT.  The following code does not work correctly if you did enter it code and loss_time was 0.

-- 
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/3182#discussion_r341808463

---

From: ianswett <[email-masked]>
Date: Sat, 02 Nov 2019 04:51:13 -0700

Closed #3182.

-- 
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/3182#event-2765186250