diff --git a/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/additional-listener-test-script.js new file mode 100644 index 000000000000..033bb077b1cd --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/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/integrations/OnUncaughtException/mimic-behaviour-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/mimic-behaviour-additional-listener-test-script.js new file mode 100644 index 000000000000..e9c53bddc1fb --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/mimic-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({ + mimicNativeBehaviour: true, + }); + } 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/integrations/OnUncaughtException/mimic-behaviour-no-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/mimic-behaviour-no-additional-listener-test-script.js new file mode 100644 index 000000000000..7b3307db4211 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/mimic-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({ + mimicNativeBehaviour: true, + }); + } 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/integrations/OnUncaughtException/no-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/no-additional-listener-test-script.js new file mode 100644 index 000000000000..fcff5962b629 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/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/integrations/OnUncaughtException/test.ts b/packages/node-integration-tests/suites/public-api/integrations/OnUncaughtException/test.ts new file mode 100644 index 000000000000..3c656443fc96 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/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 `mimicNativeBehaviour` set to true', () => { + test('should close process on uncaught error with no additional listeners registered', done => { + expect.assertions(3); + + const testScriptPath = path.resolve(__dirname, 'mimic-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-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-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test-script.js b/packages/node-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test-script.js new file mode 100644 index 000000000000..0ca2ea78c0e3 --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test-script.js @@ -0,0 +1,29 @@ +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: integrations => { + return integrations.map(integration => { + if (integration.name === 'OnUnhandledRejection') { + return new Sentry.Integrations.OnUnhandledRejection({ + mode: process.env['PROMISE_REJECTION_MODE'], + }); + } else { + return integration; + } + }); + }, +}); + +if (process.env['ATTACH_ADDITIONAL_HANDLER']) { + process.on('uncaughtException', () => { + // do nothing - this will prevent the rejected promise below from closing this process before the timeout resolves + }); +} + +setTimeout(() => { + process.stdout.write("I'm alive!"); + process.exit(0); +}, 500); + +Promise.reject(); diff --git a/packages/node-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test.ts b/packages/node-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test.ts new file mode 100644 index 000000000000..83e91d6f8c0f --- /dev/null +++ b/packages/node-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test.ts @@ -0,0 +1,36 @@ +import * as childProcess from 'child_process'; +import * as path from 'path'; + +describe('OnUnhandledRejection integration', () => { + test.each([ + { mode: 'none', additionalHandler: false, expectedErrorCode: 0 }, + { mode: 'warn', additionalHandler: false, expectedErrorCode: 0 }, + { mode: 'strict', additionalHandler: false, expectedErrorCode: 1 }, + + { mode: 'none', additionalHandler: true, expectedErrorCode: 0 }, + { mode: 'warn', additionalHandler: true, expectedErrorCode: 0 }, + { mode: 'strict', additionalHandler: true, expectedErrorCode: 1 }, + ])( + 'should cause process to exit with code $expectedErrorCode with `mode` set to $mode while having a handler attached (? - $additionalHandler)', + async ({ mode, additionalHandler, expectedErrorCode }) => { + const testScriptPath = path.resolve(__dirname, 'test-script.js'); + + await new Promise(resolve => { + childProcess.exec( + `node ${testScriptPath}`, + { + encoding: 'utf8', + env: { + PROMISE_REJECTION_MODE: mode ? String(mode) : undefined, + ATTACH_ADDITIONAL_HANDLER: additionalHandler ? String(additionalHandler) : undefined, + }, + }, + err => { + expect(err?.code).toBe(expectedErrorCode || undefined); + resolve(); + }, + ); + }); + }, + ); +}); diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index dfff26fa0675..e9c25c4e3f02 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -8,6 +8,28 @@ 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. + /** + * Whether the SDK should mimic native behaviour when a global error occurs. Default: `false` + * - `false`: The SDK will exit the process on all uncaught errors. + * - `true`: The SDK will only exit the process when there are no other 'uncaughtException' handlers attached. + */ + mimicNativeBehaviour: 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 Promise Rejection handler */ export class OnUncaughtException implements Integration { /** @@ -23,26 +45,25 @@ export class OnUncaughtException implements Integration { /** * @inheritDoc */ - public readonly handler: (error: Error) => void = this._makeErrorHandler(); + public readonly handler: (error: Error) => void = this._makeErrorHandler().bind(this); + + 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 = { + mimicNativeBehaviour: false, + ...options, + }; + } + /** * @inheritDoc */ public setupOnce(): void { - global.process.on('uncaughtException', this.handler.bind(this)); + global.process.on('uncaughtException', this.handler); } /** @@ -59,6 +80,26 @@ export class OnUncaughtException implements Integration { let onFatalError: OnFatalErrorHandler = logAndExitProcess; const client = getCurrentHub().getClient(); + // 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 shouldExitProcess: boolean = !this._options.mimicNativeBehaviour || userProvidedListenersCount === 0; + if (this._options.onFatalError) { // eslint-disable-next-line @typescript-eslint/unbound-method onFatalError = this._options.onFatalError; @@ -83,23 +124,23 @@ export class OnUncaughtException implements Integration { originalException: error, data: { mechanism: { handled: false, type: 'onuncaughtexception' } }, }); - if (!calledFatalError) { + if (!calledFatalError && shouldExitProcess) { calledFatalError = true; onFatalError(error); } }); } else { - if (!calledFatalError) { + if (!calledFatalError && shouldExitProcess) { calledFatalError = true; onFatalError(error); } } - } else if (calledFatalError) { + } else if (calledFatalError && shouldExitProcess) { // we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down IS_DEBUG_BUILD && logger.warn('uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown'); logAndExitProcess(error); - } else if (!caughtSecondError) { + } else if (!caughtSecondError && shouldExitProcess) { // 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 diff --git a/packages/node/src/integrations/onunhandledrejection.ts b/packages/node/src/integrations/onunhandledrejection.ts index 2ba9a3d8f205..74148da79ee0 100644 --- a/packages/node/src/integrations/onunhandledrejection.ts +++ b/packages/node/src/integrations/onunhandledrejection.ts @@ -4,7 +4,16 @@ import { consoleSandbox } from '@sentry/utils'; import { logAndExitProcess } from './utils/errorhandling'; -type UnhandledRejectionMode = 'none' | 'warn' | 'strict'; +interface OnUnhandledRejectionOptions { + // TODO(v8): Evaluate whether we should mimic nodes default behaviour for this integration and/or remove the mode option. + // 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. + /** + * Option deciding what to do after capturing unhandledRejection, + * that mimicks behavior of node's --unhandled-rejection flag. Default: `'warn'` + */ + mode: 'none' | 'warn' | 'strict'; +} /** Global Promise Rejection handler */ export class OnUnhandledRejection implements Integration { @@ -18,18 +27,17 @@ export class OnUnhandledRejection implements Integration { */ public name: string = OnUnhandledRejection.id; + private readonly _options: OnUnhandledRejectionOptions; + /** * @inheritDoc */ - public constructor( - private readonly _options: { - /** - * Option deciding what to do after capturing unhandledRejection, - * that mimicks behavior of node's --unhandled-rejection flag. - */ - mode: UnhandledRejectionMode; - } = { mode: 'warn' }, - ) {} + public constructor(options: Partial = {}) { + this._options = { + mode: 'warn', + ...options, + }; + } /** * @inheritDoc