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

[core] Refactored ACK window management to avoid false error reports #1888

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Mar 24, 2021

Fixes #253

This refactors the acknowledge function from Window tools, which not only returns what it was supposed to return so far as results, but also returns a status that allows to distinguish the reason as to why the ACK node could not be used this time to calculate RTT:

  • ROGUE: The node journal is completely out of tolerable range for existing ACK nodes (normally when it's from the future, or for ACK that has never been sent). IPE log should report this case.
  • OLD: One of the returning ACKACK packets was likely lost and therefore the next ACKACK swallowed this node as well (likely ACKACK packets have been reordered). Do not report this case, it's an unimportant problem. Note also that it's completely intended to remove the ACK node for which ACKACK has come together with all older nodes, even though an ACKACK could theoretically come for them - even if it happens (and results in OLD), the RTT measurement from such a node would be far from trustworthy.
  • WIPED: The node journal has been removed, although it was definitely within the range. IPE log should report this case.

Effectively, only if OK is returned should the RTT calculation be taken. If OLD, simply ignore the case (no log required), otherwise issue an IPE log message, as these situations shall never happen in a healthy code.

The body of acknowledge has been also completely rewritten for clarity and better performance. Also test cases have been provided.

@ethouris ethouris force-pushed the dev-refax-ack-window branch from f3d43c3 to a5de970 Compare May 31, 2021 14:44
@ethouris ethouris requested a review from maxsharabayko May 31, 2021 14:54
@ethouris ethouris marked this pull request as ready for review May 31, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPE: ACK node overwritten message (impact unknown)
1 participant