ValentinBELYN / icmplib

Easily forge ICMP packets and make your own ping and traceroute.
GNU Lesser General Public License v3.0
267 stars 45 forks source link

Timeout does not trigger soon enough if count > 1 #22

Closed bdraco closed 3 years ago

bdraco commented 3 years ago

I think this is the cause of https://github.com/home-assistant/core/issues/40232

The timeout is being linked to the count.

>>> import time
>>> from icmplib import ping
>>> time.time() ; ping("1.2.3.4", timeout=2, count=30) ; time.time()
1602340930.801749
<Host [1.2.3.4]>
1602341005.2304416
>>> 
>>> time.time() ; ping("1.2.3.4", timeout=2) ; time.time()
1602340720.058677
<Host [1.2.3.4]>
1602340728.365908
>>> time.time() ; ping("1.2.3.4", timeout=1) ; time.time()
1602340734.0859458
<Host [1.2.3.4]>
1602340738.0910802
>>> 
ValentinBELYN commented 3 years ago

This is the normal behavior of the library. It is also the case with the ping command integrated in your system. A ping corresponds to the sending of an ICMP request to a remote host and the receiving of an ICMP reply indicating the reachability of the host. In the event of no response from the latter, a timer is decremented, the timeout, before considering the request or the response as lost (probably because the host cannot be reached).

If you make two pings and the remote host does not respond, you wait twice for this timeout. With 30 pings, you will wait 30 times for this timeout in the event of no response. Obviously, if the host responds, this timeout is never considered.

Consequently, the timeout that you specified in parameter applies to each request and the maximum waiting time can be count * timeout in the event of no response.

ValentinBELYN commented 3 years ago

From README.md:

bdraco commented 3 years ago

Thanks for the explanation:

When I look at the man page for ping:

     -t timeout
             Specify a timeout, in seconds, before ping exits regardless of how many packets have been received.

My expectation was that it worked in the same way as -t

ping -t 1 -c 5 (times out in 1 second)

% time ping -t 1 -c 5 192.168.1.1     
PING 192.168.1.1 (192.168.1.1): 56 data bytes
Request timeout for icmp_seq 0

--- 192.168.1.1 ping statistics ---
2 packets transmitted, 0 packets received, 100.0% packet loss
ping -t 1 -c 5 192.168.1.1  0.00s user 0.00s system 0% cpu 1.006 total

ping -t 2 -c 5 (times out in 1 second)

bdraco@Js-MacBook-Pro-3 ~ % time ping -t 2 -c 5 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
Request timeout for icmp_seq 0

--- 192.168.1.1 ping statistics ---
2 packets transmitted, 0 packets received, 100.0% packet loss
ping -t 2 -c 5 192.168.1.1  0.00s user 0.01s system 0% cpu 2.013 total
bdraco@Js-MacBook-Pro-3 ~ % 

Adjusting the timeout to 1 on the Home Assistant side should resolve the issue that user is reporting.

bdraco commented 3 years ago

Adjusted here https://github.com/home-assistant/core/pull/41620

ValentinBELYN commented 3 years ago

Um... What I meant is this is my normal behavior: to tend to be sure of myself and not to check my sources when I am tired 😭.

I am really sorry. This behavior is unique to Windows and even though I am working on Linux, I thought the ping command had a similar timeout parameter.