diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/OnUncaughtException/additional-listener-test-script.js new file mode 100644 index 000000000000..033bb077b1cd --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/additional-listener-test-script.js @@ -0,0 +1,16 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', +}); + +process.on('uncaughtException', () => { + // do nothing - this will prevent the Error below from closing this process before the timeout resolves +}); + +setTimeout(() => { + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +throw new Error(); diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js new file mode 100644 index 000000000000..db68624592e5 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js @@ -0,0 +1,27 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: integrations => { + return integrations.map(integration => { + if (integration.name === 'OnUncaughtException') { + return new Sentry.Integrations.OnUncaughtException({ + exitEvenIfOtherHandlersAreRegistered: false, + }); + } else { + return integration; + } + }); + }, +}); + +process.on('uncaughtException', () => { + // do nothing - this will prevent the Error below from closing this process before the timeout resolves +}); + +setTimeout(() => { + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +throw new Error(); diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-no-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-no-additional-listener-test-script.js new file mode 100644 index 000000000000..6f27c6b5cb05 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-no-additional-listener-test-script.js @@ -0,0 +1,24 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: integrations => { + return integrations.map(integration => { + if (integration.name === 'OnUncaughtException') { + return new Sentry.Integrations.OnUncaughtException({ + exitEvenIfOtherHandlersAreRegistered: false, + }); + } else { + return integration; + } + }); + }, +}); + +setTimeout(() => { + // This should not be called because the script throws before this resolves + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +throw new Error(); diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/no-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/OnUncaughtException/no-additional-listener-test-script.js new file mode 100644 index 000000000000..fcff5962b629 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/no-additional-listener-test-script.js @@ -0,0 +1,13 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', +}); + +setTimeout(() => { + // This should not be called because the script throws before this resolves + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +throw new Error(); diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts b/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts new file mode 100644 index 000000000000..00c8459466c9 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts @@ -0,0 +1,57 @@ +import * as childProcess from 'child_process'; +import * as path from 'path'; + +describe('OnUncaughtException integration', () => { + test('should close process on uncaught error with no additional listeners registered', done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'no-additional-listener-test-script.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => { + expect(err).not.toBeNull(); + expect(err?.code).toBe(1); + expect(stdout).not.toBe("I'm alive!"); + done(); + }); + }); + + test('should close process on uncaught error when additional listeners are registered', done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'additional-listener-test-script.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => { + expect(err).not.toBeNull(); + expect(err?.code).toBe(1); + expect(stdout).not.toBe("I'm alive!"); + done(); + }); + }); + + describe('with `exitEvenIfOtherHandlersAreRegistered` set to false', () => { + test('should close process on uncaught error with no additional listeners registered', done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-no-additional-listener-test-script.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => { + expect(err).not.toBeNull(); + expect(err?.code).toBe(1); + expect(stdout).not.toBe("I'm alive!"); + done(); + }); + }); + + test('should not close process on uncaught error when additional listeners are registered', done => { + expect.assertions(2); + + const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-additional-listener-test-script.js'); + + childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => { + expect(err).toBeNull(); + expect(stdout).toBe("I'm alive!"); + done(); + }); + }); + }); +}); diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 64b0fdd6989a..722fdc2b1cf4 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -7,6 +7,30 @@ import { logAndExitProcess } from './utils/errorhandling'; type OnFatalErrorHandler = (firstError: Error, secondError?: Error) => void; +interface OnUncaughtExceptionOptions { + // TODO(v8): Evaluate whether we should switch the default behaviour here. + // Also, we can evaluate using https://nodejs.org/api/process.html#event-uncaughtexceptionmonitor per default, and + // falling back to current behaviour when that's not available. + /** + * Controls if the SDK should register a handler to exit the process on uncaught errors: + * - `true`: The SDK will exit the process on all uncaught errors. + * - `false`: The SDK will only exit the process when there are no other `uncaughtException` handlers attached. + * + * Default: `true` + */ + exitEvenIfOtherHandlersAreRegistered: boolean; + + /** + * This is called when an uncaught error would cause the process to exit. + * + * @param firstError Uncaught error causing the process to exit + * @param secondError Will be set if the handler was called multiple times. This can happen either because + * `onFatalError` itself threw, or because an independent error happened somewhere else while `onFatalError` + * was running. + */ + onFatalError?(firstError: Error, secondError?: Error): void; +} + /** Global Exception handler */ export class OnUncaughtException implements Integration { /** @@ -24,24 +48,23 @@ export class OnUncaughtException implements Integration { */ public readonly handler: (error: Error) => void = this._makeErrorHandler(); + private readonly _options: OnUncaughtExceptionOptions; + /** * @inheritDoc */ - public constructor( - private readonly _options: { - /** - * Default onFatalError handler - * @param firstError Error that has been thrown - * @param secondError If this was called multiple times this will be set - */ - onFatalError?(firstError: Error, secondError?: Error): void; - } = {}, - ) {} + public constructor(options: Partial = {}) { + this._options = { + exitEvenIfOtherHandlersAreRegistered: true, + ...options, + }; + } + /** * @inheritDoc */ public setupOnce(): void { - global.process.on('uncaughtException', this.handler.bind(this)); + global.process.on('uncaughtException', this.handler); } /** @@ -66,6 +89,27 @@ export class OnUncaughtException implements Integration { onFatalError = client.getOptions().onFatalError as OnFatalErrorHandler; } + // Attaching a listener to `uncaughtException` will prevent the node process from exiting. We generally do not + // want to alter this behaviour so we check for other listeners that users may have attached themselves and adjust + // exit behaviour of the SDK accordingly: + // - If other listeners are attached, do not exit. + // - If the only listener attached is ours, exit. + const userProvidedListenersCount = global.process + .listeners('uncaughtException') + .reduce((acc, listener) => { + if ( + listener.name === 'domainUncaughtExceptionClear' || // as soon as we're using domains this listener is attached by node itself + listener === this.handler // filter the handler we registered ourselves) + ) { + return acc; + } else { + return acc + 1; + } + }, 0); + + const processWouldExit = userProvidedListenersCount === 0; + const shouldApplyFatalHandlingLogic = this._options.exitEvenIfOtherHandlersAreRegistered || processWouldExit; + if (!caughtFirstError) { const hub = getCurrentHub(); @@ -82,47 +126,53 @@ export class OnUncaughtException implements Integration { originalException: error, data: { mechanism: { handled: false, type: 'onuncaughtexception' } }, }); - if (!calledFatalError) { + if (!calledFatalError && shouldApplyFatalHandlingLogic) { calledFatalError = true; onFatalError(error); } }); } else { - if (!calledFatalError) { + if (!calledFatalError && shouldApplyFatalHandlingLogic) { calledFatalError = true; onFatalError(error); } } - } else if (calledFatalError) { - // we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down - __DEBUG_BUILD__ && - logger.warn('uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown'); - logAndExitProcess(error); - } else if (!caughtSecondError) { - // two cases for how we can hit this branch: - // - capturing of first error blew up and we just caught the exception from that - // - quit trying to capture, proceed with shutdown - // - a second independent error happened while waiting for first error to capture - // - want to avoid causing premature shutdown before first error capture finishes - // it's hard to immediately tell case 1 from case 2 without doing some fancy/questionable domain stuff - // so let's instead just delay a bit before we proceed with our action here - // in case 1, we just wait a bit unnecessarily but ultimately do the same thing - // in case 2, the delay hopefully made us wait long enough for the capture to finish - // two potential nonideal outcomes: - // nonideal case 1: capturing fails fast, we sit around for a few seconds unnecessarily before proceeding correctly by calling onFatalError - // nonideal case 2: case 2 happens, 1st error is captured but slowly, timeout completes before capture and we treat second error as the sendErr of (nonexistent) failure from trying to capture first error - // note that after hitting this branch, we might catch more errors where (caughtSecondError && !calledFatalError) - // we ignore them - they don't matter to us, we're just waiting for the second error timeout to finish - caughtSecondError = true; - setTimeout(() => { - if (!calledFatalError) { - // it was probably case 1, let's treat err as the sendErr and call onFatalError - calledFatalError = true; - onFatalError(firstError, error); - } else { - // it was probably case 2, our first error finished capturing while we waited, cool, do nothing + } else { + if (shouldApplyFatalHandlingLogic) { + if (calledFatalError) { + // we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down + __DEBUG_BUILD__ && + logger.warn( + 'uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown', + ); + logAndExitProcess(error); + } else if (!caughtSecondError) { + // two cases for how we can hit this branch: + // - capturing of first error blew up and we just caught the exception from that + // - quit trying to capture, proceed with shutdown + // - a second independent error happened while waiting for first error to capture + // - want to avoid causing premature shutdown before first error capture finishes + // it's hard to immediately tell case 1 from case 2 without doing some fancy/questionable domain stuff + // so let's instead just delay a bit before we proceed with our action here + // in case 1, we just wait a bit unnecessarily but ultimately do the same thing + // in case 2, the delay hopefully made us wait long enough for the capture to finish + // two potential nonideal outcomes: + // nonideal case 1: capturing fails fast, we sit around for a few seconds unnecessarily before proceeding correctly by calling onFatalError + // nonideal case 2: case 2 happens, 1st error is captured but slowly, timeout completes before capture and we treat second error as the sendErr of (nonexistent) failure from trying to capture first error + // note that after hitting this branch, we might catch more errors where (caughtSecondError && !calledFatalError) + // we ignore them - they don't matter to us, we're just waiting for the second error timeout to finish + caughtSecondError = true; + setTimeout(() => { + if (!calledFatalError) { + // it was probably case 1, let's treat err as the sendErr and call onFatalError + calledFatalError = true; + onFatalError(firstError, error); + } else { + // it was probably case 2, our first error finished capturing while we waited, cool, do nothing + } + }, timeout); // capturing could take at least sendTimeout to fail, plus an arbitrary second for how long it takes to collect surrounding source etc } - }, timeout); // capturing could take at least sendTimeout to fail, plus an arbitrary second for how long it takes to collect surrounding source etc + } } }; }