Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always save the packet size and return early #4409

Merged
merged 4 commits into from
Nov 26, 2020

Conversation

ianswett
Copy link
Contributor

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

FIxes #4408

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

FIxes #4408
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
ianswett and others added 2 commits November 25, 2020 09:40
Co-authored-by: Jana Iyengar <jri.ietf@gmail.com>
Only in_flight packets are added to lost_packets, so adding an assert to improve readability.
@ianswett
Copy link
Contributor Author

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

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@janaiyengar janaiyengar merged commit 3610811 into master Nov 26, 2020
@janaiyengar janaiyengar deleted the ianswett-bytes_in_flight_in_flight branch November 26, 2020 00:08
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -recovery labels Dec 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing bytes from flight is a bit confusing
3 participants