search

qbit / node-pushover

Pushover notifications for node.js (JavaScript,NodeJS)
ISC License
163 stars 24 forks source link

Crash when Pushover API responds with HTML #25

Open sulkaharo opened 5 years ago

sulkaharo commented 5 years ago

I just had an app crash and the stack trace indicates the issue is with the node-pushover library not handling the API not responding with a JSON object. Add some defensive code somewhere to check if the response actually parses as JSON? :)

houtenbos commented 5 years ago

I have had the same problem twice today: SyntaxError: Unexpected token < in JSON at position 0 at JSON.parse () at Pushover.errors (/var/app/bogota/node_modules/pushover-notifications/lib/pushover.js:134:14) at IncomingMessage. (/var/app/bogota/node_modules/pushover-notifications/lib/pushover.js:241:12) at IncomingMessage.emit (events.js:198:15) at endReadableNT (_stream_readable.js:1139:12) at processTicksAndRejections (internal/process/task_queues.js:81:17)

sulkaharo commented 5 years ago

Sure, I'm at least not expecting their service to have 100% uptime, but the library should not crash on a 5xx response. I just forked this and added some defensive try/catching in hopes that this stops the crash https://github.com/sulkaharo/node-pushover

sulkaharo commented 5 years ago

Made a PR to wrap the crashing JSON conversion https://github.com/qbit/node-pushover/pull/26

qbit commented 5 years ago

Thanks! I have requested a slight change. I will publish a new version once that is resolved (it's a trivial change, so if I don't hear back later today I will just add it and publish anyway :D)!

xtvdata commented 4 years ago

Hello, it looks like there is another case related to this issue that brings down the application.

Error: JSON parsing failed at Pushover.errors (/app/node_modules/pushover-notifications/lib/pushover.js:137:13) at IncomingMessage. (/app/node_modules/pushover-notifications/lib/pushover.js:249:12) at IncomingMessage.emit (events.js:187:15) at endReadableNT (_stream_readable.js:1094:12) at process._tickCallback (internal/process/next_tick.js:63:19)

With the latest code (v1.2.1):

IMHO, more in general to have the Pushover.prototype.errors() throw means that it would be necessary to encapsulate every piece of code that is making use of that function in a try {} catch() {} structure to avoid potential crash of the application. Example: also at line 171 in case of error there could possibly be another crash.

P.S.: I've managed to see the result of the API that generates the crash:

<head><title>429 Too Many Requests</title></head>
<body>
<center><h1>429 Too Many Requests</h1></center>
<hr><center>nginx</center>
</body>
</html>

I'm still way below the cap therefore it's probably an overload issue on the API side. I believe that pushover-notifications should manage this case specifically (wait some time and retry and/or log some kind of error, or returning a specific error). The case should be easy to intercepted since the IncomingMessage (the "res" object of request(o, function (res) {...}) ) object should contain the following properties:

  statusCode: 429,
  statusMessage: 'Too Many Requests',
qbit commented 4 years ago

@jonasgroendahl or @xtvdata would either of you be able to share a snippet of how you are triggering this?

Is it just 5 requests fired off at the same time?

jonasgroendahl commented 4 years ago

yes basically 5 requests or so at the same time will trigger a crash of node. doesn't matter if i include image or not.

qbit commented 4 years ago

can you guys test 2f2fcb6a77778c301a136c660845c71231600ba7 ? It works in my tests (trying to fix travis but it doesn't like my tokens)

qbit commented 4 years ago

Pushed 1.2.2.

xtvdata commented 4 years ago

Still having an error with latest version:

TypeError: this.onerror is not a function
    at Pushover.errors (.../node_modules/pushover-notifications/lib/pushover.js:137:12)

If I may, I’d suggest, in case of return status 429 from the API server, to throw or return (via callback) a specific error so that either this module or the application code can manage this specific error code with a delayed retry (otherwise the notification would be lost). Best would be to be able to set a “retry delay” to avoid overloading the API server, and a “maximum number of retries” (so that if the API server is down, or there are infrastructure connection issues, the process wouldn’t be left there retrying indefinitely).

Moreover, in case of implementing delayed retries in the module, I’d also suggest to set the notification date-time in order to keep the right timestamp on the notification even if it has been delayed.

Anyway the main point is that, since the instance method .send() already returns via callback an error code, the method .send() should never Throw, but all errors should be handled or passed over via the error object in the callback (so that the application will handle the error). Otherwise, to be sure that he application wont crash, any implementation should manage errors twice: one based on error in callback and one with a try-catch.

qbit commented 4 years ago

Are you setting onerror to a function? That error suggests you are not.

Here is a snippet from my test where it's being set:

var p = new Push({
  user: process.env['PUSHOVER_USER'],
  token: process.env['PUSHOVER_TOKEN'],
  update_sounds: false,
  debug: true,
  onerror: function (err, res) {
    if (res.statusCode == 429) {
      process.exit(0)
    }
  }
})
xtvdata commented 4 years ago

Indeed that was generated by a missing function in my test case. However, I was not expecting the error there. Since I was calling the send method I was expecting the error to be notified in the send callback.

Here is a practical use case, in which (if I'm not mistaken) the current behavior would make the case difficult to manage.

When an error like 429 happens, I'd like to retry sending the message, lets say, 3 time (each retry delayed 5-10 min from the previous one).

If such an error would have been returned in the error object of the send method, it would be easy to manage a retry procedure since the contents of the notification are in the context of the call to send.

Instead, if error is reported in the Push object instance, the details of the notification might not be in context, forcing the implementation of a retry mechanism to manage some kind of queue (possibly persisted on somewhere kind of additional storage system, such as a DB) and a separate background notification process.

sulkaharo commented 4 years ago

@qbit ping - can you please change the Pushover object creation to mandate the onerror setting. The way the current release works is, if an error happens and onerror is not defined, the app will crash due to trying to call an undefined function. Related, the documentation for the library indicates the function is optional, but it's not really. :)