Eneris / push-receiver

A library to subscribe to GCM/FCM and receive notifications within a node process.
https://medium.com/@MatthieuLemoine/my-journey-to-bring-web-push-support-to-node-and-electron-ce70eea1c0b0
MIT License
15 stars 15 forks source link

any uncaught error in onNotification cause push-receiver to become stale and ignore subsequent notifications #22

Closed baryosef-loops closed 3 weeks ago

baryosef-loops commented 1 month ago

Encountered an issue where any uncaught errors thrown within the onNotification event handler cause the push notification receiver to become unresponsive to new notifications. This behavior seems to stem from errors not being properly caught and handled within the EventEmitter used as super (parent class).

As a result, the instance becomes stale silently, without any notice or debugged information, and no further push notifications will be handled by that same instance, effectively causing the receiver to "freeze" and ignore any subsequent notifications.

Steps to Reproduce:

  1. Set up the push-receiver according to the documentation.
  2. Implement an onNotification handler that intentionally throws an error. For example:
    
    pushReceiver.onNotification(() => {
    throw new Error("Test error");
    // or 
    console.log(myObject.someProp) // if myObject is null for some reason
    // or
    callSomeFunctionWithThrowError() 

});


Observe that after the error is thrown, the receiver no longer processes incoming notifications.

### Expected Behavior:
PushReceiver should be able to recover from such errors and never become stale.
Errors should be handled internally in the same way as socket errors.
Actual Behavior:
When an error is thrown within the onNotification handler, the receiver stops processing any new notifications, becoming unresponsive.

### Possible Solution:
[Implementing error handling within the EventEmitter](https://nodejs.org/api/events.html#error-events) for the onNotification event could potentially resolve half of this issue. The other half might be deeper in the call stack, depending on the hierarchy of the onNotification internal implementation. Additionally, providing a way to catch, observe, or log any errors instead of silently becoming a stale instance would be beneficial. This could involve handling errors internally or providing a callback to observe errors.

### Current Workaround
Use `Try-Catch` block within any on('...') handler, eg:

pushReceiver.onNotification(() => { try { throw new Error("Test error"); } catch { ... } });



### Environment:
Library version: 4.1.3
Node.js version: v20.3.1
Operating System: macOS

This issue is critical for applications relying on continuous push notifications for functionality, as any uncaught error can disrupt the notification flow, impacting user experience.

BTW - thank you for the work, this lib as great value to many apps!
Eneris commented 1 month ago

Thanks for report. I'll check it out.

Eneris commented 1 month ago

I've just released new version 4.1.4 which should address this issue. https://www.npmjs.com/package/@eneris/push-receiver/v/4.1.4 Can you please verify?

baryosef-loops commented 1 month ago

@Eneris thank you, I will test during this week. What is the eventual solution? Can I add 'error' listeners? Will they be logged to console? Or else?

Eneris commented 4 weeks ago

@Eneris thank you, I will test during this week. What is the eventual solution? Can I add 'error' listeners? Will they be logged to console? Or else?

I have added custom EventEmitter wrapper that wraps any listeners in try..catch and prints the error with console.error. I may add an error event later but that could possibly end up with circular deadlocks if used inproperly

Eneris commented 3 weeks ago

Ok since there is no response, I take this as resolved