danielzzz / node-ping

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

Exception "TypeError Object.probe(ping.lib:ping-promise)" #122

Closed Apollon77 closed 3 years ago

Apollon77 commented 4 years ago

Hi,

we use ping ^0.2.3 in one project and we have Sentry as crash reporting tool integrated. We got one exception from a users system about an exception. Maybe you want to enhance error handing? :-)

Details: https://sentry.iobroker.net/share/issue/6444e16a395448bcabb48324d3da0ce4/

TypeError: Cannot read property 'on' of undefined
  File "/opt/iobroker/node_modules/iobroker.shelly/node_modules/ping/lib/ping-promise.js", line 73, col 17, in Object.probe
    ping.stdout.on('data', function (data) {
  File "/opt/iobroker/node_modules/iobroker.shelly/node_modules/ping/lib/ping-sys.js", line 38, col 17, in Object.probe
    return ping.probe(addr, _config).then(function (res) {
  File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 29, col 14, in Promise
    ping.sys.probe(host, (isAlive) => {
  ?, in new Promise
  File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 28, col 10, in pingAsync
    return new Promise((resolve, reject) => {
  File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 459, col 23, in CoAPClient.httpIoBrokerState
    let alive = await pingAsync(this.getIP());
  File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 525, col 56, in Timeout.timerid.setTimeout [as _onTimeout]
    this.timerid = setTimeout(async () => await this.httpIoBrokerState(), this.polltime);
  File "timers.js", line 436, col 11, in ontimeout
  File "timers.js", line 300, col 5, in tryOnTimeout
  File "timers.js", line 263, col 5, in listOnTimeout
mondwan commented 4 years ago

I am not familiar with sentry. What kind of user system is running on?

We r using system ping, which is not suitable on running in browser at all. R u running our library on browser?

在 2020年6月24日週三 14:10,Ingo Fischer notifications@github.com 寫道:

Hi,

we use ping ^0.2.3 in one project and we have Sentry as crash reporting tool integrated. We got one exception from a users system about an exception. Maybe you want to enhance error handing? :-)

Details: https://sentry.iobroker.net/share/issue/6444e16a395448bcabb48324d3da0ce4/

TypeError: Cannot read property 'on' of undefined File "/opt/iobroker/node_modules/iobroker.shelly/node_modules/ping/lib/ping-promise.js", line 73, col 17, in Object.probe ping.stdout.on('data', function (data) { File "/opt/iobroker/node_modules/iobroker.shelly/node_modules/ping/lib/ping-sys.js", line 38, col 17, in Object.probe return ping.probe(addr, _config).then(function (res) { File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 29, col 14, in Promise ping.sys.probe(host, (isAlive) => { ?, in new Promise File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 28, col 10, in pingAsync return new Promise((resolve, reject) => { File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 459, col 23, in CoAPClient.httpIoBrokerState let alive = await pingAsync(this.getIP()); File "/opt/iobroker/node_modules/iobroker.shelly/lib/coap.js", line 525, col 56, in Timeout.timerid.setTimeout [as _onTimeout] this.timerid = setTimeout(async () => await this.httpIoBrokerState(), this.polltime); File "timers.js", line 436, col 11, in ontimeout File "timers.js", line 300, col 5, in tryOnTimeout File "timers.js", line 263, col 5, in listOnTimeout

— 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/issues/122, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA5YBI4C546MMJNDSFX3NDLRYGKEZANCNFSM4OGKURSQ .

Apollon77 commented 4 years ago

Sentry is just a crash reporting too that reports crashes from user systems to us. It was a "linux" (os.platform) system with nodejs v10.15.2

Apollon77 commented 4 years ago

In fact it is about "ping.stdout" is undefined in this case ... so it seems that something went wrong before which lead to this case. reason unknown ... but maybe it is a good idea (with knowing that it indeed happened to at least 1 user) to check for it. :-)

mondwan commented 4 years ago

If ping.stdout is none, there are some problems on cp.spawn, which is a standard library child_process. Can you take a look what's going on on cp.spawn there?

Apollon77 commented 4 years ago

When you google then it can happen that stdout is undefined ... you just need to check such cases before assuming it is there ...

mondwan commented 4 years ago

According to the manual, stdout will be none only if stdio option is set to be none. Since there is only 1 report regarding to this issue, it is reason for me to guess the output of cp.spawn in your work station.

Let's say I simply test this condition, and reject the promise with error message "Missing stdout". It does not resolve the problem in your case.

Apollon77 commented 4 years ago

It will allow to catch the error and return it to the user ... much better thenan async uncatchable exception :-)

mondwan commented 4 years ago

It is fine to capture such kind of errors. But, we still cannot find out why stdout is none. If we know why it is none, we may have a better solution than just simply capture such kind of errors

Apollon77 commented 4 years ago

I completely agree, but I have no idea. According to Sentry it affects 3 users at the moment

mondwan commented 3 years ago

Hey, @Apollon77 . I have wrapped the exception as requested. Combining the improvement in #128 , it may solves your issue. Please check the fix on master branch

Apollon77 commented 3 years ago

I checked the code - that should work. When released as NPM version can include it and we can see if the error happens again (btw I never had it myself, we just got crash reports from our sentry from other users)

mondwan commented 3 years ago

Patches are available on 0.4.0. Please create a new issue if there are any new problems. Thanks