Crash when Pushover API responds with HTML #25

sulkaharo · 2019-07-01T16:21:37Z

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? :)

Jul 01 09:02:23 app/web.1: <!doctype html>
Jul 01 09:02:23 app/web.1: ^
Jul 01 09:02:23 app/web.1: SyntaxError: Unexpected token < in JSON at position 0
Jul 01 09:02:23 app/web.1: at JSON.parse ()
Jul 01 09:02:23 app/web.1: at Pushover.errors (/app/node_modules/pushover-notifications/lib/pushover.js:134:14)
Jul 01 09:02:23 app/web.1: at IncomingMessage. (/app/node_modules/pushover-notifications/lib/pushover.js:241:12)
Jul 01 09:02:23 heroku/web.1: State changed from up to crashed
Jul 01 09:02:23 app/web.1: at IncomingMessage.emit (events.js:203:15)
Jul 01 09:02:23 app/web.1: at endReadableNT (_stream_readable.js:1129:12)
Jul 01 09:02:23 app/web.1: at process._tickCallback (internal/process/next_tick.js:63:19)

houtenbos · 2019-10-26T07:09:09Z

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 · 2019-10-26T10:18:59Z

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 · 2019-10-26T13:27:32Z

Made a PR to wrap the crashing JSON conversion #26

qbit · 2019-10-27T15:10:16Z

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 · 2020-01-22T14:09:04Z

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):

the module probably receives back from API something unparsable and
throws an Error at line 137,
however at line 249 the error seems to be not handled properly and the app crashes.

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 · 2020-01-30T22:28:20Z

@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 · 2020-01-30T22:31:18Z

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 · 2020-01-30T23:34:29Z

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

qbit · 2020-01-30T23:58:17Z

Pushed 1.2.2.

xtvdata · 2020-02-13T09:22:18Z

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 · 2020-02-13T13:50:51Z

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 · 2020-02-14T09:23:18Z

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 · 2020-09-30T06:08:31Z

@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. :)

qbit mentioned this issue Jan 30, 2020

Error: JSON parsing failed #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crash when Pushover API responds with HTML #25

Crash when Pushover API responds with HTML #25

sulkaharo commented Jul 1, 2019

houtenbos commented Oct 26, 2019

sulkaharo commented Oct 26, 2019

sulkaharo commented Oct 26, 2019

qbit commented Oct 27, 2019

xtvdata commented Jan 22, 2020 •

edited

Loading

qbit commented Jan 30, 2020

jonasgroendahl commented Jan 30, 2020

qbit commented Jan 30, 2020

qbit commented Jan 30, 2020

xtvdata commented Feb 13, 2020 •

edited

Loading

qbit commented Feb 13, 2020

xtvdata commented Feb 14, 2020

sulkaharo commented Sep 30, 2020

Crash when Pushover API responds with HTML #25

Crash when Pushover API responds with HTML #25

Comments

sulkaharo commented Jul 1, 2019

houtenbos commented Oct 26, 2019

sulkaharo commented Oct 26, 2019

sulkaharo commented Oct 26, 2019

qbit commented Oct 27, 2019

xtvdata commented Jan 22, 2020 • edited Loading

qbit commented Jan 30, 2020

jonasgroendahl commented Jan 30, 2020

qbit commented Jan 30, 2020

qbit commented Jan 30, 2020

xtvdata commented Feb 13, 2020 • edited Loading

qbit commented Feb 13, 2020

xtvdata commented Feb 14, 2020

sulkaharo commented Sep 30, 2020

xtvdata commented Jan 22, 2020 •

edited

Loading

xtvdata commented Feb 13, 2020 •

edited

Loading