feat(node): Add option to OnUncaughtException integration that allows mimicking native uncaught error exit behaviour

[lforst]

This PR introduces an option (exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered) that allows users to to mimic the native node exit behaviour on errors, in the way that the integration won't exit by itself when there are additional OnUncaughtException handlers registered.

Original implementation: #5176 (thank you @ibc)

[mydea]

LGTM - as previously discussed offline, the option name and some of the uncaught exception code is generally not ideal, but we can/should revisit this for v8 or similar. Great work!

[Lms24]

Awesome work man! I have some logaf-l suggestions but we can revisit them ofc also later

[Nikoms]

I have a little question about this one: If I understand correctly exitEvenIfOtherHandlersAreRegistered is true by default? Which means that it will do a process.exit even if we manage the event?

We migrated from 6.* to 7.18 and we suddenly had a lot of pods killed because of it. Is there an easy way to remove that behavior?

I saw that we can do :

integrations: integrations => {
    return integrations.map(integration => {
      if (integration.name === 'OnUncaughtException') {
        return new Sentry.Integrations.OnUncaughtException({
          exitEvenIfOtherHandlersAreRegistered: false,
        });
      } else {
        return integration;
      }
    });
  },

Isn't it dangerous to bypass the default behavior and call process.exit in a library?

Thank you :)

[AbhiPrasad]

Hey @Nikoms

Which means that it will do a process.exit even if we manage the event

Yes you are correct.

As per the Node docs if an unhandled rejection is triggered the process should be exited: https://nodejs.org/api/process.html#warning-using-uncaughtexception-correctly. In this case, the default behavior is to exit the process, and if you register a custom handler, you should also be exiting the process. Since Sentry is registering a custom handler, we are exiting the process as a result.

The correct use of 'uncaughtException' is to perform synchronous cleanup of allocated resources (e.g. file descriptors, handles, etc) before shutting down the process. It is not safe to resume normal operation after 'uncaughtException'.

To restart a crashed application in a more reliable way, whether 'uncaughtException' is emitted or not, an external monitor should be employed in a separate process to detect application failures and recover or restart as needed.

We added this option to allow users to override this behaviour, although we don't recommend it since it's not recommended by the Node standard library itself.

[Nikoms]

Hey @AbhiPrasad, thank you for your explanation. Sorry if I'm challenging this a bit 😁

I think it would be fair to say in red somewhere "sentry can exit the process for you by default". Or disabling this feature by default, because it sounds like a BC break but was added in a minor if I'm not wrong.

I'm 100 % agree that we should all follow the recommendation of node. In that case, we can safely imagine that if there is another handler, it will exit the process, so sentry doesn't have to (talking about exitEvenIfOtherHandlersAreRegistered here)

[Nikoms]

Ok i just found that it's because of the "7.12.0" release, not this because of this commit :/ Sorry. Previously my "uncaughtException" was never called, not it is. express is catching the error for me in the middleware, not anymore. I'm not sure why (found: because of #5627 :D)