Subject: [quicwg/base-drafts] Always save the packet size and return early (#4409)

From: ianswett <[email-masked]>
Date: Tue, 24 Nov 2020 18:09:50 -0800

When the packet is not in_flight, return early instead of not saving the size earlier.

FIxes #4408
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Always save the packet size and return early

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Tue, 24 Nov 2020 18:11:08 -0800

@ianswett pushed 1 commit.

2a626247c7f564ed17522ea2ee0ba4009b070237  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4409/files/77217e144e6af02768644f473df4d4c596765460..2a626247c7f564ed17522ea2ee0ba4009b070237

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 24 Nov 2020 19:55:29 -0800

@janaiyengar requested changes on this pull request.

This is an improvement; thanks for working on it! You do need to check for `in_flight` in `OnPacketsLost()` as well where `sent_bytes` is subtracted from `bytes_in_flight`.

> @@ -1740,6 +1739,8 @@ OnPacketsAcked(acked_packets):
     OnPacketAcked(acked_packet)
 
 OnPacketAcked(acked_packet):
+  if (!ack_packet.in_flight):

```suggestion
  if (!acked_packet.in_flight):
```

-- 
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/4409#pullrequestreview-538137020

---

From: ianswett <[email-masked]>
Date: Wed, 25 Nov 2020 06:40:51 -0800

a435847b8d88d24563fb6df35790e5cc6057b7d0  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4409/files/2a626247c7f564ed17522ea2ee0ba4009b070237..a435847b8d88d24563fb6df35790e5cc6057b7d0

---

From: ianswett <[email-masked]>
Date: Wed, 25 Nov 2020 06:45:20 -0800

bffe2820703e8a29667241cc6547c763851affa9  Add an assert

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4409/files/a435847b8d88d24563fb6df35790e5cc6057b7d0..bffe2820703e8a29667241cc6547c763851affa9

---

From: ianswett <[email-masked]>
Date: Wed, 25 Nov 2020 06:46:20 -0800

We already have a check to only add in_flight packets to lost_packets, but I added an assert to hopefully make that clearer.

-- 
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/4409#issuecomment-733750266

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 25 Nov 2020 16:09:03 -0800

@janaiyengar approved this pull request.

I think symmetry here would have been clearer -- if all lost packets were in the list, similar to acked packets, and if the check for inflight was at the time of removing it from bytes_in_flight. That does make the code for OnPacketsLost more complicated though, since you have to do an early return if none of the packets lost were in flight. On balance, I think the assert is good enough -- thanks!

-- 
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/4409#pullrequestreview-538914384

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 25 Nov 2020 16:09:11 -0800

Merged #4409 into master.

-- 
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/4409#event-4040244187