Subject: [quicwg/base-drafts] don't apply congestion penalty for lost ack-only packets (#1223)

From: Marten Seemann <[email-masked]>
Date: Sat, 17 Mar 2018 16:18:16 +0000 (UTC)

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

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

-- Commit Summary --

  * don't apply congestion penalty for lost ack-only packets

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1223.patch
https://github.com/quicwg/base-drafts/pull/1223.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/1223

---

From: ianswett <[email-masked]>
Date: Sat, 17 Mar 2018 15:37:25 -0700

ianswett commented on this pull request.

Two comments.

> @@ -834,19 +834,19 @@ DetectLostPackets(largest_acked):
   foreach (unacked < largest_acked.packet_number):
     time_since_sent = now() - unacked.time_sent
     delta = largest_acked.packet_number - unacked.packet_number
-    if (time_since_sent > delay_until_lost):
-      lost_packets.insert(unacked)
-    else if (delta > reordering_threshold)
+    if (time_since_sent > delay_until_lost ||
+      delta > reordering_threshold):

I think the convention is to indent the second line of the condition 2 more than the first?

>        lost_packets.insert(unacked)
+      sent_packets.remove(unacked.packet_number)
+      if (!unacked.is_ack_only):
+        lost_packets.insert(unacked)

You already inserted this packet 3 lines above.  Did you mean to remove that line?

-- 
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/1223#pullrequestreview-104780136

---

From: Marten Seemann <[email-masked]>
Date: Sun, 18 Mar 2018 08:57:45 +0000 (UTC)

marten-seemann commented on this pull request.

Isn't that what I did?

-- 
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/1223#discussion_r175281765

---

From: Marten Seemann <[email-masked]>
Date: Sun, 18 Mar 2018 09:01:14 +0000 (UTC)

@marten-seemann pushed 1 commit.

2a7f8aa  Ian's review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1223/files/41afebeed40ed2565f898d74efa155d14986f8f1..2a7f8aa719aa6876bec34b3f7fa419bb5e9a174f

---

From: Marten Seemann <[email-masked]>
Date: Sun, 18 Mar 2018 09:01:32 +0000 (UTC)

Yes. Fixed.

-- 
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/1223#discussion_r175281847

---

From: ianswett <[email-masked]>
Date: Sun, 18 Mar 2018 02:24:59 -0700

> @@ -834,19 +834,18 @@ DetectLostPackets(largest_acked):

I think the d needs to line up with the 't' of time_since_sent based on looking at OnPacketAcked.

The current indention is confusing, since the delta > reordering_threshold blends into the code that follows.

-- 
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/1223#pullrequestreview-104790758

---

From: Marten Seemann <[email-masked]>
Date: Sun, 18 Mar 2018 02:31:22 -0700

1950689  indent

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1223/files/2a7f8aa719aa6876bec34b3f7fa419bb5e9a174f..1950689ce78c568d03f985a8a7c535854e4544fb

---

From: Marten Seemann <[email-masked]>
Date: Sun, 18 Mar 2018 09:38:08 +0000 (UTC)

Got it. Fixed.

-- 
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/1223#discussion_r175282780

---

From: ianswett <[email-masked]>
Date: Sun, 18 Mar 2018 02:45:25 -0700

ianswett approved this pull request.

-- 
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/1223#pullrequestreview-104791299

---

From: ianswett <[email-masked]>
Date: Sun, 18 Mar 2018 02:47:31 -0700

> @@ -1039,11 +1038,11 @@ Invoked by loss detection from DetectLostPackets when new packets
 are detected lost.
 
 ~~~
-   OnPacketsLost(lost_packets):
+   OnPacketsLost(lost_packet):

Did you accidentally change lost_packets to lost_packet?

-- 
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/1223#pullrequestreview-104791358

---

From: Marten Seemann <[email-masked]>
Date: Sun, 18 Mar 2018 09:53:55 +0000 (UTC)

are detected lost.
 
 ~~~

Yeah, I shouldn't have made any changes there. Fixed it and squashed everything.

-- 
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/1223#discussion_r175283188

---

From: ianswett <[email-masked]>
Date: Sun, 18 Mar 2018 12:12:44 +0000 (UTC)

-- 
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/1223#pullrequestreview-104795001

---

From: ianswett <[email-masked]>
Date: Sun, 18 Mar 2018 05:12:50 -0700

Merged #1223.

-- 
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/1223#event-1527206459