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

Examples have diverged from the definitions in out-of-order packets section #183

Closed
aochagavia opened this issue May 1, 2023 · 5 comments

Comments

@aochagavia
Copy link
Contributor

aochagavia commented May 1, 2023

I'm implementing the ACK frequency draft and it looks like the examples have diverged from the definitions in the section titled "Response to Out-of-Order Packets".

When looking at the text, the definition of "Unreported Missing" says they are "Packets with packet numbers between the Largest Unacked and Largest Acked that have not yet been received" (emphasis mine). When looking at the examples, however, it looks like the definition is slightly modified to include all missing packets lower than Largest Unacked (i.e. even when they are lower than Largest Acked).

Below I have worked out the provided examples, to show how the mismatch looks in practice. Notice how, in both examples, an ACK is sent in the last line even though there are no unreported missing packets between Largest Unacked and Largest Acked.

Which one is correct? The text or the examples?

Example 1

reordering_threshold = 3

largest_unacked = 0, largest_acked = 0, unreported_missing = [], gap = 0
Receive 1
largest_unacked = 1, largest_acked = 0, unreported_missing = [], gap = 0
Receive 3
largest_unacked = 3, largest_acked = 0, unreported_missing = [2], gap = 1
Receive 4
largest_unacked = 4, largest_acked = 0, unreported_missing = [2], gap = 2
Receive 5
largest_unacked = 5, largest_acked = 0, unreported_missing = [2], gap = 3
===> Send ACK, because gap >= reordering_threshold
largest_unacked = 5, largest_acked = 5, unreported_missing = [], gap = 0
Receive 8
largest_unacked = 8, largest_acked = 5, unreported_missing = [6, 7], gap = 2
Receive 9
largest_unacked = 9, largest_acked = 5, unreported_missing = [6, 7], gap = 3
===> Send ACK, because gap >= reordering_threshold
largest_unacked = 9, largest_acked = 9, unreported_missing = [], gap = 0
Receive 10
largest_unacked = 10, largest_acked = 9, unreported_missing = [], gap = 0
===> The draft says: "Send ACK because of 7", which means it considers 7 to be unreported missing!

Example 2

reordering_threshold = 5

largest_unacked = 0, largest_acked = 0, unreported_missing = [], gap = 0
Receive 1
largest_unacked = 1, largest_acked = 0, unreported_missing = [], gap = 0
Receive 3
largest_unacked = 3, largest_acked = 0, unreported_missing = [2], gap = 1
Receive 5
largest_unacked = 5, largest_acked = 0, unreported_missing = [2, 4], gap = 3
Receive 6
largest_unacked = 6, largest_acked = 0, unreported_missing = [2, 4], gap = 4
Receive 7
largest_unacked = 7, largest_acked = 0, unreported_missing = [2, 4], gap = 5
===> Send ACK, because gap >= reordering_threshold
largest_unacked = 7, largest_acked = 7, unreported_missing = [], gap = 0
Receive 8
largest_unacked = 8, largest_acked = 7, unreported_missing = [], gap = 0
Receive 9
largest_unacked = 9, largest_acked = 7, unreported_missing = [], gap = 0
===> The draft says: "Send ACK because of 4", which means it considers 4 to be unreported missing!
@mirjak
Copy link
Contributor

mirjak commented Jun 7, 2023

Thanks for flagging this. It has been challenging to get the text write that's why we included examples. We will revisit the text again and try to get it straight!

@huitema
Copy link

huitema commented Jul 11, 2023

I think this is somewhat related to #213.

@ianswett
Copy link
Collaborator

ianswett commented Sep 9, 2023

Thanks for filing this!

I believe #222, which fixed #213, caused the text to line up with the examples. @aochagavia Can you verify that and if so close this issue?

@aochagavia
Copy link
Contributor Author

Thanks for fixing it :)

I don't have the bandwith right now to check, so if you think the issue is fixed feel free to close the issue. I'll also give a heads up to the folks developing Quinn, so if there is any ambiguity left they'll probably find it.

@ianswett
Copy link
Collaborator

Thanks!

Ralith added a commit to quinn-rs/quinn that referenced this issue Sep 24, 2023
Ralith added a commit to quinn-rs/quinn that referenced this issue Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants