Sentry Node: do not exit process if the app added its own 'uncaughtException' or 'unhandledRejection' listener

[ibc]

• Fixes Can't prevent Sentry SDK from exiting the process even with onFatalError #1661

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Ref: https://getsentry.atlassian.net/browse/WEB-939

[lforst]

Hi, I took the liberty of rebasing this PR and cleaning it up a bit. Thanks for your contribution! We'll make sure to mention you in the changelog.

I fixed a small bug where domains would interfere with the listener count. Additionally, we decided it would be best to put this behaviour behind an option. We can reevaluate at a later point in time (ie, when we do a major bump) if we want to alter the default behaviour. Currently this seems to risky because people might depend on this behaviour in their applications.

The OnUnhandledRejection integration doesn't need adjustment because users can already control the desired behaviour with the mode option.

This decision might be a bit unsatisfying at the moment. And trust me, I agree with you that the SDK should never alter behaviour like this - bugs me a lot. For the moment however, we need to consider downstream users that already depend on this behaviour. I'll make sure to revisit this when we have another major bump.

[ibc]

Thanks for your feedback.

Regarding "users may already depend on current behavior" I fail to understand it. For me it's the opposite: our app relies on Node's behavior (if you set an "uncaughtException" listener, Node guarantees that it won't exit the process when an uncaught exception happens). Then we integrated Sentry and such a behavior changed dramatically. IMHO it makes more sense that users rely on Node's official documentation than in a wrong behavior of a 3rd party library (do not take me wrong).

[lforst]

Yeah I see your point. However, changing the default might break current users, which is a valid concern for us. As I said - we might revisit the default behaviour for the next major version.

[lforst]

@AbhiPrasad can I get another pair of 👀 on this since I added a bunch of logic?

[lforst]

Did a little bit more thinking on this change in general. I will close this PR. Currently it doesn't add any particular value to have an option for this behaviour. Users can just provide their own handler in the onFatalError option of OnUncaughtException and set a mode on the OnUnhandledRejection integration. At some point, preferably next major bump, I'd love to change the default behaviour to match nodes without any configuration needed. We can revisit this PR when we get to it.

[dzlandis]

@lforst, can you please provide an example of how to do this:

Users can just provide their own handler in the onFatalError option of OnUncaughtException and set a mode on the OnUnhandledRejection integration.

This part of the documentation specifically states that:

The onFatalError option is meant to perform a cleanup before the process exits, not fully prevent it from exiting.

Hence, there is no way, from what I can see, for me to resolve my issue which relates to #1661.
I think this should be reopened...

[lforst]

@dzlandis We will currently not reopen this PR since changing the default behaviour is a breaking change and we're not planning on doing a major update right now.

Additionally, providing options to mimic node's native behaviour is also not really adding much since users have already all the tools they need to mimic the behaviour themselves via the OnUncaughtException integration's onFatalError option and via the OnUnhandledRejection integration's mode option.

I guess you would do it like the following: Do this instead.

import * as Sentry from "@sentry/node";

Sentry.init({
  // Other options ...

  integrations: (defaultIntegrations) => {
    return defaultIntegrations.map((integration) => {
      if (integration.name === "OnUncaughtException") {
        return new Sentry.Integrations.OnUncaughtException({
          onFatalError: (error, errorPotentiallyThrownInOnFatalError) => {
            // Do whatever you like here. Quit the app, keep it alive, log the Error...
            // If this option is provided, the SDK will not quit the application.
            // Keep in mind however, that if you want all the error events that have built up to be sent to Sentry,
            // you need to await:
            // await Sentry.getCurrentHub().getClient()?.close()
          },
        });
      } else if (integration.name === "OnUnhandledRejection") {
        return new Sentry.Integrations.OnUnhandledRejection({ mode: "none" }); // chose between "none", "warn" and "strict"
      } else {
        return integration;
      }
    });
  },
});

[i-tu]

@lforst The sample you provide does not work on Sentry 7.13.3 – onFatalError will run but not prevent the process exiting. OnUncaughtException needs to be completely disabled: #1661 (comment)

I appreciate that you don't want to make a breaking change here 👍 I wish that same standard was applied to #5627 which was a breaking change and caused production issues for a ton of people, including myself.

[lforst]

@i-tu I think you're right. Thanks for letting me know. I also updated my comment.

As for the breaking change: I see that this issue and #5627 are somewhat similar. With #5627 we didn't immediately see the impact it would have (everybody makes mistakes) but we decided not to roll back. Here it is quite clear to us and we make the conscious decision to go through with it until we do a major bump. Hope that sounds reasonable - thank you for the feedback! :)

[i-tu]

@lforst I completely understand. Thank you for the new option in #6137 – I really appreciate it!