danielzzz / node-ping

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

Misleading values in returned object when the cable is unplugged or the host doesn't exist #133

Closed lucamarogna closed 3 years ago

lucamarogna commented 3 years ago

Writing these tests:

console.log( await ping.promise.probe('fakehostdoesntexist.com') );
// Manually unplug the cable...
console.log( await ping.promise.probe('google.com') );

both result in:

{
  host: 'unknown',
  alive: false,
  output: '',
  time: 'unknown',
  times: [],
  min: 'unknown',
  max: 'unknown',
  avg: 'unknown',
  stddev: 'unknown',
  packetLoss: 'unknown'
}

(the latter happen only when the network cable is unplugged)

I think it should have at least the host property setted to the actual host pinged. Not sure for other properties as ping implementation may vary depending on platforms (I'm on Ubuntu 18.04.3).

mondwan commented 3 years ago

Since node-ping relies on underlying system command implementation, data reflects what it can be fetch from the system's command.

In your case, since you have unplugged the cable before running probe, I guess ping returns only error message about network device is gone or etc. No hostname can be fetched as as result.

Actually, both cases fall into same fact that node-ping cannot fetch messages from the underlying system command. We can do not much in this scenario.

lucamarogna commented 3 years ago

Ok that's totally fine.

For me, it would be better to have the host value because I'm running several promises in parallel (Promise.all()) and I didn't know which has failed (e.g. "fakehostdoesntexist.com"), but I do catch your point.

Thank you for your response and explanation.

mondwan commented 3 years ago

For your case, how about adding a key, say, userInputHost in the response object.

Therefore, we have around 3 keys talking about hosts but with subtle difference:

Hopefully, it can address your issue

mondwan commented 3 years ago

You can checkout this new implementation in the master branch on github.

HughDai commented 3 years ago

For your case, how about adding a key, say, userInputHost in the response object.

Therefore, we have around 3 keys talking about hosts but with subtle difference:

  • userInputHost: As title, host inputted from user
  • host: Host parsed from the system command
  • numeric_host: Host parsed, if possible, from the system command in a numeric format

Hopefully, it can address your issue

I don't find this commit in the latest npm version v0.4.0 . Could you please check it, thanks a lot.

mondwan commented 3 years ago

as marked in #136, hopefully, I tag it wrongly. Try 0.4.1. It should be correct now

hugh @.***> 於 2021年4月7日週三 下午8:03寫道:

For your case, how about adding a key, say, userInputHost in the response object.

Therefore, we have around 3 keys talking about hosts but with subtle difference:

  • userInputHost: As title, host inputted from user
  • host: Host parsed from the system command
  • numeric_host: Host parsed, if possible, from the system command in a numeric format

Hopefully, it can address your issue

I don't find this commit in the latest npm version v0.4.0 . Could you please check it, thanks a lot.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/danielzzz/node-ping/issues/133#issuecomment-814858072, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5YBIY4GCN7RYHVTIV4WVTTHRCZDANCNFSM4VBJN6DQ .