sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

The `onUnhandledRejectionIntegration` does not exit gracefully in strict mode

Open WesCossick opened this issue 1 year ago • 1 comments

Is there an existing issue for this?

  • [X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
  • [X] I have reviewed the documentation https://docs.sentry.io/
  • [X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.5.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

const sentryConfig: Sentry.NodeOptions = {
	dsn: process.env.SENTRY_DSN,
	environment: process.env.ENVIRONMENT,
	integrations: [
		Sentry.onUnhandledRejectionIntegration({
			mode: 'strict',
		}),
	],
};

Sentry.init(sentryConfig);

Steps to Reproduce

Trigger an unhandled promise rejection while using the onUnhandledRejectionIntegration in mode: 'strict'.

Expected Result

For the app to exit gracefully.

Actual Result

When Sentry's onUnhandledRejectionIntegration catches an unhandled promise rejection in strict mode, it is currently designed to exit immediately:

https://github.com/getsentry/sentry-javascript/blob/005f40df3e9e2715b9f634d4490d1e42760be61d/packages/node/src/utils/errorhandling.ts#L34

This does not allow the app to shut things down and exit gracefully, because it bypasses any SIGTERM listeners. The code should really be designed like so:

if (process.listenerCount('SIGTERM')) {
  global.process.kill(process.pid, 'SIGTERM');
} else {
  global.process.exit(1);
}

This will detect if there are any SIGTERM listeners, and if so, rely on those to handle shut down and exit.

WesCossick avatar May 28 '24 17:05 WesCossick

Note, a temporary fix for anyone else encountering this would be to not use mode: 'strict' and instead add your own unhandledRejection listener after calling Sentry.init():

process.on('unhandledRejection', () => {
  if (process.listenerCount('SIGTERM')) {
    process.kill(process.pid, 'SIGTERM');
  } else {
    process.exit(1);
  }
});

WesCossick avatar May 28 '24 17:05 WesCossick

Hi, thanks for writing in. Would you mind walking me through your thought process?

This is how I currently look at this issue: You manually set mode: 'strict' meaning you told the SDK to exit the process within its unhandledRejection handler. You also mentioned SIGTERM listeners not being called. As far as I understand, SIGTERM listeners are not invoked by Node when it crashes through unhandled promise rejections. I verified with the following snippet:

process.once('SIGTERM', () => {
  console.log('sigterm received'); // not called
});

setTimeout(() => {
  console.log('healthy'); // not called
}, 4000);

Promise.reject();

As far as I can tell, you simply shouldn't set the mode to 'strict' if you don't want the SDK to exit, and instead use 'warn' (which is the default) or 'none'.

lforst avatar May 31 '24 08:05 lforst

I can see your reasoning, and while I'd normally say adhering to the behavior of a platform like Node.js is a good idea, when it comes to exiting gracefully and signal handling, Node.js isn't the best example; there are a number of flaws with how Node.js handles signal listeners and graceful shutdown.

Essentially, my reasoning is that if a program registers SIGTERM listeners, then it wants to clean things up when it's time to exit. So using process.kill(process.pid, 'SIGTERM') to shut down is the way to trigger a graceful exit. While it deviates from Node.js's behavior, I'd argue that this is a better exiting strategy because it allows for cleanup tasks to be executed.

The workaround I suggested avoids using strict, but it introduces slightly extra complexity that wouldn't be necessary if Sentry's SDK terminated more gracefully.

WesCossick avatar May 31 '24 20:05 WesCossick

Thanks for elaborating. I think we will not change this behaviour. I don't intend to dismiss your point but there is some history 😄 From my last research on whether we can be more accurate here, I don't think there is a way to exactly match Node.js's exit behaviour when intending run IO (in our case to flush data). We certainly do not want to "improve" Node.js' behaviours - that is up to our users.

I'll close this to keep our issue stream clean but feel free to reach out if you have any questions or concerns!

lforst avatar Jun 03 '24 05:06 lforst