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.

Sign up for GitHub

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

Jump to bottom

multi_ping retry not working correctly #23

Open
BriceSchaffner opened this issue Apr 11, 2019 · 1 comment
Open

multi_ping retry not working correctly #23

BriceSchaffner opened this issue Apr 11, 2019 · 1 comment

Comments

@BriceSchaffner
Copy link

When a request times out, a retry is correctly sent with a new ID but the receive function always looks for the old ID. This result by sending as many retry as specified in the multi_ping function and returning a failed ping as soon as at least one packet loss occurred, even if the first retry was successful.
BriceSchaffner pushed a commit to BriceSchaffner/multi-ping that referenced this issue Apr 11, 2019
refs romana#23: fixed multi_ping retry
5926a28 
Now the multi_ping retry functionality works correctly.
The issue was that an ICMP retry packet was sent with a new ID, but the
receive function was still checking packets using old IDs.

Now the remainings_id is reinitialized with the new ids during each sends.
This way the receive function always used the latest ids for packet
matching.

The source address of the packet has also been added to the packet
matching to make the request/reply match more robust.
BriceSchaffner pushed a commit to BriceSchaffner/multi-ping that referenced this issue Apr 11, 2019
refs romana#23: Added number of retry in result
6094a78 
Now the number of retry are added to the result dictionary.
@BriceSchaffner BriceSchaffner mentioned this issue Apr 11, 2019
Open
echodusty pushed a commit to echodusty/multi-ping that referenced this issue Apr 28, 2019
1. Add 0.3 seconds delay after each ICMP send.
3417d17 
2. Try to fix for issue romana#23. Detail:
   Change self._remaining_ids to list.
   Clear the self._remaining_ids list when retry accoured.
   Append new unique ID to self._remaining_ids every times run.
@brandyn
Copy link

brandyn commented Jun 24, 2021

Fwiw, I just hit this bug and fixed it before finding this thread.

I eliminated _remaining_ids and replaced it with _remaining_addrs, which resulted in notably simpler code overall (net reduction in lines of code, and I think the logic is more clear this way). It fixes the bug mentioned above, in a way that continues to listen for replies to any of the pings sent by a host and registers the first one received. The changes are very simple and clear, so easy to visually vet.

I can provide the changes if anyone is interested, but if you need to me submit them formally you'll have to hold my hand through the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@brandyn @BriceSchaffner