search

danielzzz / node-ping

a poor man's ping library (using udp scanning) for node
MIT License
336 stars 105 forks source link

use timeout option when timeout config option is set #101

Closed sa-linetco closed 5 years ago

sa-linetco commented 5 years ago

fixes #100

mondwan commented 5 years ago

Thanks for your patch. But, that's in purpose to use -w instead of -W.

Let me know if I am wrong.

According to my experience, the option you suggest cannot guarantee the command exits within that period.

Usually, I don't care how many pings have been sent. But, I do care there is a response (no matter success or not) within a certain time frame.

That's why I use the deadline option. It returns immediately after getting first successful response. Otherwise, it is guaranteed to be closed for seconds I defined.

在 2019年8月22日週四 18:44，sa-linetco notifications@github.com 寫道：

fixes #100 https://github.com/danielzzz/node-ping/issues/100

You can view, comment on, or merge this pull request online at:

https://github.com/danielzzz/node-ping/pull/101 Commit Summary

  • use timeout option when timeout config option is set

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/danielzzz/node-ping/pull/101?email_source=notifications&email_token=AA5YBI7ZFMSQCC4XGMGTIKLQFZUZ7A5CNFSM4IOTRXE2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HGX6OZQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5YBI3LBORB5ONK4SIJ53DQFZUZ7ANCNFSM4IOTRXEQ .

sa-linetco commented 5 years ago

With the timeout option set, ping also immediately returns on the first successful ping.

time /bin/ping -n -w 5 -c 1 192.168.133.33
PING 192.168.133.33 (192.168.133.33) 56(84) bytes of data.
64 bytes from 192.168.133.33: icmp_seq=1 ttl=64 time=2.35 ms

--- 192.168.133.33 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 2.358/2.358/2.358/0.000 ms

real    0m0,007s
user    0m0,001s
sys 0m0,004s
time /bin/ping -n -W 5 -c 1 192.168.133.33
PING 192.168.133.33 (192.168.133.33) 56(84) bytes of data.
64 bytes from 192.168.133.33: icmp_seq=1 ttl=64 time=5.11 ms

--- 192.168.133.33 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 5.110/5.110/5.110/0.000 ms

real    0m0,010s
user    0m0,001s
sys 0m0,004s

As described in #100 -W also guarantees that the ping process exits after the given timeout.

The difference between timeout and deadline is that deadline will ignore the count parameter in case the ping requests are not being answered. But as ping is called with -n (or -c) in the module, I expect that only the given number of ping requests is sent (which is also what I'd expect after reading the documentation: * @property {number} min_reply - Exit after sending number of ECHO_REQUEST.

sa-linetco commented 5 years ago

If you want to conserve the possibility to set the deadline option on the javascript side, I'd suggest adding a deadline config option to the ping module. Translating timeout to the deadline ping option seems counterintuitive to me (especially as there is a timeout option available).

mondwan commented 5 years ago

I have done some experiments regarding to the deadline, and timeout option.

In your scenario, one ping packet, both options guarantee the exit time of command ping. But, timeout cannot do so if number of ping packets are more than 1, and the host is not reachable.

time ping -n -w 10 -c 10 1.2.3.4
PING 1.2.3.4 (1.2.3.4) 56(84) bytes of data.

--- 1.2.3.4 ping statistics ---
10 packets transmitted, 0 received, 100% packet loss, time 9209ms

real    0m10.001s
user    0m0.002s
sys 0m0.000s

time ping -n -W 10 -c 10 1.2.3.4
PING 1.2.3.4 (1.2.3.4) 56(84) bytes of data.

--- 1.2.3.4 ping statistics ---
10 packets transmitted, 0 received, 100% packet loss, time 9218ms

real    0m19.222s
user    0m0.000s
sys 0m0.003s

For host is reachable case, the result sames as the unreachable case mentioned above.

To conclude, I agree with your changes on timeout option (-w to -W), but we do need a new deadline option, which is linux specific, and write it down in the README FAQ section for this subtle usage.

Would you mind to help me to implement them?

sa-linetco commented 5 years ago

Would you mind to help me to implement them?

Sure thing! As I do not have a mac available, could you please make sure that the mac version also inherits the unix ping options of -w for deadline and -W for timeout? On the windows parameter builder I'd suggest to log a warning if deadline option is used.

mondwan commented 5 years ago

Thanks.

For the MAC one, I will check that later on. For the window one, I agree with your solution.

在 2019年8月23日週五 15:23，sa-linetco notifications@github.com 寫道：

Would you mind to help me to implement them? Sure thing! As I do not have a mac available, could you please make sure that the mac version also inherits the unix ping options of -w for deadline and -W for timeout? On the windows parameter builder I'd suggest to log a warning if deadline option is used.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/danielzzz/node-ping/pull/101?email_source=notifications&email_token=AA5YBI2UOYRYRQ7SITXYNUTQF6F67A5CNFSM4IOTRXE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD47LSPQ#issuecomment-524204350, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5YBIY24LYNZPRSUXGKKN3QF6F67ANCNFSM4IOTRXEQ .

sa-linetco commented 5 years ago

For now I implemented the same behaviour for mac as I did for windows. When the deadline option is set, an error is thrown.

sa-linetco commented 5 years ago

Another fact I found out while working on this: -w on windows and -W on linux do not show the same behaviour. I think we should change the documentation regarding the configuration object accordingly.

Example (windows, unreachable ip):
> Measure-Command {start-process ping -argumentlist "-w 7000 -n 3 172.29.14.32" -wait}
TotalSeconds      : 23,5091395
Example (windows, reachable ip)
> Measure-Command {start-process ping -argumentlist "-w 7000 -n 3 192.168.133.33" -wait}
TotalSeconds      : 3,0480599
Example (linux, unreachable ip):
:~$ time ping -W 7 -c 3 172.29.14.32
PING 172.29.14.32 (172.29.14.32) 56(84) bytes of data.

--- 172.29.14.32 ping statistics ---
3 packets transmitted, 0 received, 100% packet loss, time 2038ms

real    0m9,040s
user    0m0,001s
sys 0m0,000s
Example (linux, reachable ip)
time ping -W 7 -c 3 192.168.133.33
PING 192.168.133.33 (192.168.133.33) 56(84) bytes of data.
64 bytes from 192.168.133.33: icmp_seq=1 ttl=64 time=2.65 ms
64 bytes from 192.168.133.33: icmp_seq=2 ttl=64 time=3.65 ms
64 bytes from 192.168.133.33: icmp_seq=3 ttl=64 time=2.67 ms

--- 192.168.133.33 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2004ms
rtt min/avg/max/mdev = 2.658/2.996/3.652/0.465 ms

real    0m2,012s
user    0m0,006s
sys 0m0,000s
mondwan commented 5 years ago

Thanks. These are great, and high quality patch.

Conclusion

Type MAC OS Ping Linux Ping Window ping
deadline -t (unit sec) -w (unit sec) NA
timeout -W (unit msec) -W (unit sec) -w (unit msec)

Experiments from MAC OS

Below are experiments on MAC OS

Not reachable cases

deadline option

MacBook-Air-4:~$ time ping -t 10 -c 15 1.2.3.4
PING 1.2.3.4 (1.2.3.4): 56 data bytes
Request timeout for icmp_seq 0
Request timeout for icmp_seq 1
Request timeout for icmp_seq 2
Request timeout for icmp_seq 3
Request timeout for icmp_seq 4
Request timeout for icmp_seq 5
Request timeout for icmp_seq 6
Request timeout for icmp_seq 7
Request timeout for icmp_seq 8

--- 1.2.3.4 ping statistics ---
10 packets transmitted, 0 packets received, 100.0% packet loss

real    0m10.008s
user    0m0.002s
sys 0m0.003s

timeout option

MacBook-Air-4:~$ time ping -W 1000 -c 15 1.2.3.4
PING 1.2.3.4 (1.2.3.4): 56 data bytes
Request timeout for icmp_seq 0
Request timeout for icmp_seq 1
Request timeout for icmp_seq 2
Request timeout for icmp_seq 3
Request timeout for icmp_seq 4
Request timeout for icmp_seq 5
Request timeout for icmp_seq 6
Request timeout for icmp_seq 7
Request timeout for icmp_seq 8
Request timeout for icmp_seq 9
Request timeout for icmp_seq 10
Request timeout for icmp_seq 11
Request timeout for icmp_seq 12
Request timeout for icmp_seq 13

--- 1.2.3.4 ping statistics ---
15 packets transmitted, 0 packets received, 100.0% packet loss

real    0m16.040s
user    0m0.002s
sys 0m0.005s

Reachable case

deadline

MacBook-Air-4:~$ time ping -t 1 -c 5 8.8.8.8
PING 8.8.8.8 (8.8.8.8): 56 data bytes
64 bytes from 8.8.8.8: icmp_seq=0 ttl=55 time=6.148 ms

--- 8.8.8.8 ping statistics ---
2 packets transmitted, 1 packets received, 50.0% packet loss
round-trip min/avg/max/stddev = 6.148/6.148/6.148/0.000 ms

real    0m1.007s
user    0m0.002s
sys 0m0.003s

Timeout

MacBook-Air-4:~$ time ping -W 1000 -c 5 8.8.8.8
PING 8.8.8.8 (8.8.8.8): 56 data bytes
64 bytes from 8.8.8.8: icmp_seq=0 ttl=55 time=5.667 ms
64 bytes from 8.8.8.8: icmp_seq=1 ttl=55 time=6.099 ms
64 bytes from 8.8.8.8: icmp_seq=2 ttl=55 time=5.860 ms
64 bytes from 8.8.8.8: icmp_seq=3 ttl=55 time=14.086 ms
64 bytes from 8.8.8.8: icmp_seq=4 ttl=55 time=6.324 ms

--- 8.8.8.8 ping statistics ---
5 packets transmitted, 5 packets received, 0.0% packet loss
round-trip min/avg/max/stddev = 5.667/7.607/14.086/3.247 ms

real    0m4.022s
user    0m0.002s
sys 0m0.003s
sa-linetco commented 5 years ago 
* Below are conclusion so far on this issue. Would you mind to implement the MAC OS side as described in below table?

Sure thing, I'll get on it later today.

sa-linetco commented 5 years ago 
* For the window's ping behaviour, I cannot notices the differences you mentioned. Would you mind to elaborate the differences so that we can see how to update the document accordingly?

Timeout: 7 Count: 3 Time for the ping process to exit:

unreachable IP reachable IP
linux 9 2
windows 23 3

The documentation for the configuration objects states that timeout determines the time for the ping process to exit, which is not what timeout stands for. See:

  • @property {number} timeout - Time duration, in seconds, for ping command to exit

My suggestion for the timeout option:

 * @property {number} timeout - Timeout in seconds for each ping request. Behaviour varies between platforms. Check platform ping documentation for more information.
mondwan commented 5 years ago

Great. Thanks for your patch. I have link up the discussions here in the README.

If there are no problems at all, I will merge this MR in a day or 2

sa-linetco commented 5 years ago

Hi @mondwan , are there any problems that arose with this version which need addressing?

mondwan commented 5 years ago

Ah, I forgot to press the button. Thanks for the remind.

sa-linetco commented 5 years ago

Thanks for the merge! Do you have any info on when to expect a release on npm?

mondwan commented 5 years ago

No idea. You need to contact danielzzz directly since I don't have access on npm

在 2019年9月17日週二 15:39，sa-linetco notifications@github.com 寫道：

Thanks for the merge! Do you have any info on when to expect a release on npm?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/danielzzz/node-ping/pull/101?email_source=notifications&email_token=AA5YBI3TLFP4EZWPPRSH6PDQKCCS3A5CNFSM4IOTRXE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD63TSSQ#issuecomment-532101450, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5YBIYT3VXTPHIEEQN2P3LQKCCS3ANCNFSM4IOTRXEQ .