From 8922b5be01019cd5f31dc7c6f21ea7e3d2244413 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 12:28:06 +0000 Subject: [PATCH 1/7] feat(node): Add option to `OnUncaughtException` integration that allows mimicing native exit behaviour --- .../additional-listener-test-script.js | 16 +++++ ...haviour-additional-listener-test-script.js | 27 ++++++++ ...iour-no-additional-listener-test-script.js | 24 +++++++ .../no-additional-listener-test-script.js | 13 ++++ .../public-api/OnUncaughtException/test.ts | 57 +++++++++++++++ .../src/integrations/onuncaughtexception.ts | 69 +++++++++++++++---- 6 files changed, 192 insertions(+), 14 deletions(-) create mode 100644 packages/node-integration-tests/suites/public-api/OnUncaughtException/additional-listener-test-script.js create mode 100644 packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-behaviour-additional-listener-test-script.js create mode 100644 packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-behaviour-no-additional-listener-test-script.js create mode 100644 packages/node-integration-tests/suites/public-api/OnUncaughtException/no-additional-listener-test-script.js create mode 100644 packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts 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-behaviour-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/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/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/OnUncaughtException/mimic-behaviour-no-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/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/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/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..3c656443fc96 --- /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 `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/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 64b0fdd6989a..498794da9c1c 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -7,6 +7,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 Exception handler */ export class OnUncaughtException implements Integration { /** @@ -24,19 +46,18 @@ 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 = { + mimicNativeBehaviour: false, + ...options, + }; + } + /** * @inheritDoc */ @@ -58,6 +79,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; @@ -82,23 +123,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 __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 From 55ed724b86bd1af1bdc99bb4f14e38a47c4944a2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 12:32:10 +0000 Subject: [PATCH 2/7] fix(nextjs): Do not exit process when errors bubble up while additional `uncaughtException`-handlers are registered --- packages/nextjs/src/index.server.ts | 5 +++++ packages/node/src/integrations/onuncaughtexception.ts | 2 ++ 2 files changed, 7 insertions(+) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index 285c6eb1599e..dc52fa156e68 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -118,6 +118,11 @@ function addServerIntegrations(options: NextjsOptions): void { }); integrations = addOrUpdateIntegration(defaultRewriteFramesIntegration, integrations); + const nativeBehaviourOnUncaughtException = new Integrations.OnUncaughtException(); + integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, { + _options: { mimicNativeBehaviour: true }, + }); + if (hasTracingEnabled(options)) { const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true }); integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, { diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 498794da9c1c..34d05f12dbf7 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -7,6 +7,7 @@ import { logAndExitProcess } from './utils/errorhandling'; type OnFatalErrorHandler = (firstError: Error, secondError?: Error) => void; +// CAREFUL: Please think twice before updating the way _options looks because the Next.js SDK depends on it in `index.server.ts` 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 @@ -46,6 +47,7 @@ export class OnUncaughtException implements Integration { */ public readonly handler: (error: Error) => void = this._makeErrorHandler(); + // CAREFUL: Please think twice before updating the way _options looks because the Next.js SDK depends on it in `index.server.ts` private readonly _options: OnUncaughtExceptionOptions; /** From 71bcfe06176f298fad18bc98a3f3ada9cc1f9f14 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 13:50:03 +0000 Subject: [PATCH 3/7] cleanup --- ...aviour-additional-listener-test-script.js} | 2 +- ...our-no-additional-listener-test-script.js} | 2 +- .../public-api/OnUncaughtException/test.ts | 6 +- .../src/integrations/onuncaughtexception.ts | 96 ++++++++++--------- 4 files changed, 57 insertions(+), 49 deletions(-) rename packages/node-integration-tests/suites/public-api/OnUncaughtException/{mimic-behaviour-additional-listener-test-script.js => mimic-native-behaviour-additional-listener-test-script.js} (89%) rename packages/node-integration-tests/suites/public-api/OnUncaughtException/{mimic-behaviour-no-additional-listener-test-script.js => mimic-native-behaviour-no-additional-listener-test-script.js} (88%) diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-behaviour-additional-listener-test-script.js b/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js similarity index 89% rename from packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-behaviour-additional-listener-test-script.js rename to packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js index e9c53bddc1fb..0ca7611319d0 100644 --- a/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-behaviour-additional-listener-test-script.js +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js @@ -6,7 +6,7 @@ Sentry.init({ return integrations.map(integration => { if (integration.name === 'OnUncaughtException') { return new Sentry.Integrations.OnUncaughtException({ - mimicNativeBehaviour: true, + exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false, }); } else { return integration; diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-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 similarity index 88% rename from packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-behaviour-no-additional-listener-test-script.js rename to packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-no-additional-listener-test-script.js index 7b3307db4211..cd4a4f1009b2 100644 --- a/packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-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 @@ -6,7 +6,7 @@ Sentry.init({ return integrations.map(integration => { if (integration.name === 'OnUncaughtException') { return new Sentry.Integrations.OnUncaughtException({ - mimicNativeBehaviour: true, + exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false, }); } else { return integration; diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts b/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts index 3c656443fc96..fe114deda31c 100644 --- a/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts @@ -28,11 +28,11 @@ describe('OnUncaughtException integration', () => { }); }); - describe('with `mimicNativeBehaviour` set to true', () => { + describe('with `exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered` 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-behaviour-no-additional-listener-test-script.js'); + 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(); @@ -45,7 +45,7 @@ describe('OnUncaughtException integration', () => { 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'); + const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-additional-listener-test-script.js'); childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => { expect(err).toBeNull(); diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 498794da9c1c..5f184006a480 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -12,11 +12,11 @@ interface OnUncaughtExceptionOptions { // 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` + * Whether the SDK should mimic native behaviour when a global error occurs. Default: `true` * - `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; + exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: boolean; /** * This is called when an uncaught error would cause the process to exit. @@ -53,7 +53,7 @@ export class OnUncaughtException implements Integration { */ public constructor(options: Partial = {}) { this._options = { - mimicNativeBehaviour: false, + exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: true, ...options, }; } @@ -62,7 +62,7 @@ export class OnUncaughtException implements Integration { * @inheritDoc */ public setupOnce(): void { - global.process.on('uncaughtException', this.handler.bind(this)); + global.process.on('uncaughtException', this.handler); } /** @@ -79,6 +79,14 @@ export class OnUncaughtException implements Integration { let onFatalError: OnFatalErrorHandler = logAndExitProcess; const client = getCurrentHub().getClient(); + if (this._options.onFatalError) { + // eslint-disable-next-line @typescript-eslint/unbound-method + onFatalError = this._options.onFatalError; + } else if (client && client.getOptions().onFatalError) { + // eslint-disable-next-line @typescript-eslint/unbound-method + 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: @@ -97,15 +105,9 @@ export class OnUncaughtException implements Integration { } }, 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; - } else if (client && client.getOptions().onFatalError) { - // eslint-disable-next-line @typescript-eslint/unbound-method - onFatalError = client.getOptions().onFatalError as OnFatalErrorHandler; - } + const processWouldExit = userProvidedListenersCount === 0; + const shouldApplyFatalHandlingLogic = + this._options.exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered || processWouldExit; if (!caughtFirstError) { const hub = getCurrentHub(); @@ -123,47 +125,53 @@ export class OnUncaughtException implements Integration { originalException: error, data: { mechanism: { handled: false, type: 'onuncaughtexception' } }, }); - if (!calledFatalError && shouldExitProcess) { + if (!calledFatalError && shouldApplyFatalHandlingLogic) { calledFatalError = true; onFatalError(error); } }); } else { - if (!calledFatalError && shouldExitProcess) { + if (!calledFatalError && shouldApplyFatalHandlingLogic) { calledFatalError = true; onFatalError(error); } } - } else if (calledFatalError && shouldExitProcess) { - // 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 && 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 - // - 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 + } } }; } From f4056866b6913d7b23e67533e8af048e9ff46900 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 13:57:04 +0000 Subject: [PATCH 4/7] Change option name --- packages/nextjs/src/index.server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index dc52fa156e68..0b8c82c2b710 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -120,7 +120,7 @@ function addServerIntegrations(options: NextjsOptions): void { const nativeBehaviourOnUncaughtException = new Integrations.OnUncaughtException(); integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, { - _options: { mimicNativeBehaviour: true }, + _options: { exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false }, }); if (hasTracingEnabled(options)) { From 74ab0bd7719c3711dea9dc704bf3dadea82908cd Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 14:22:39 +0000 Subject: [PATCH 5/7] Rename option --- ...mic-native-behaviour-additional-listener-test-script.js | 2 +- ...-native-behaviour-no-additional-listener-test-script.js | 2 +- .../suites/public-api/OnUncaughtException/test.ts | 2 +- packages/node/src/integrations/onuncaughtexception.ts | 7 +++---- 4 files changed, 6 insertions(+), 7 deletions(-) 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 index 0ca7611319d0..db68624592e5 100644 --- 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 @@ -6,7 +6,7 @@ Sentry.init({ return integrations.map(integration => { if (integration.name === 'OnUncaughtException') { return new Sentry.Integrations.OnUncaughtException({ - exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false, + exitEvenIfOtherHandlersAreRegistered: false, }); } else { return integration; 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 index cd4a4f1009b2..6f27c6b5cb05 100644 --- 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 @@ -6,7 +6,7 @@ Sentry.init({ return integrations.map(integration => { if (integration.name === 'OnUncaughtException') { return new Sentry.Integrations.OnUncaughtException({ - exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false, + exitEvenIfOtherHandlersAreRegistered: false, }); } else { return integration; diff --git a/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts b/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts index fe114deda31c..00c8459466c9 100644 --- a/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts +++ b/packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts @@ -28,7 +28,7 @@ describe('OnUncaughtException integration', () => { }); }); - describe('with `exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered` set to false', () => { + describe('with `exitEvenIfOtherHandlersAreRegistered` set to false', () => { test('should close process on uncaught error with no additional listeners registered', done => { expect.assertions(3); diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 5f184006a480..457a6d6259eb 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -16,7 +16,7 @@ interface OnUncaughtExceptionOptions { * - `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. */ - exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: boolean; + exitEvenIfOtherHandlersAreRegistered: boolean; /** * This is called when an uncaught error would cause the process to exit. @@ -53,7 +53,7 @@ export class OnUncaughtException implements Integration { */ public constructor(options: Partial = {}) { this._options = { - exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: true, + exitEvenIfOtherHandlersAreRegistered: true, ...options, }; } @@ -106,8 +106,7 @@ export class OnUncaughtException implements Integration { }, 0); const processWouldExit = userProvidedListenersCount === 0; - const shouldApplyFatalHandlingLogic = - this._options.exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered || processWouldExit; + const shouldApplyFatalHandlingLogic = this._options.exitEvenIfOtherHandlersAreRegistered || processWouldExit; if (!caughtFirstError) { const hub = getCurrentHub(); From 05bfb5efed0e4de40d64cc655b8d265b38b588a5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 14:25:12 +0000 Subject: [PATCH 6/7] Rephrase JSDoc --- packages/node/src/integrations/onuncaughtexception.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 457a6d6259eb..722fdc2b1cf4 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -12,9 +12,11 @@ interface OnUncaughtExceptionOptions { // 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: `true` - * - `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. + * 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; From 85faa2ae9688c1c2b74b93b8e946d984b868364f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 4 Nov 2022 14:37:10 +0000 Subject: [PATCH 7/7] Rename option --- packages/nextjs/src/index.server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index 0b8c82c2b710..a61e49fe758f 100644 --- a/packages/nextjs/src/index.server.ts +++ b/packages/nextjs/src/index.server.ts @@ -120,7 +120,7 @@ function addServerIntegrations(options: NextjsOptions): void { const nativeBehaviourOnUncaughtException = new Integrations.OnUncaughtException(); integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, { - _options: { exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false }, + _options: { exitEvenIfOtherHandlersAreRegistered: false }, }); if (hasTracingEnabled(options)) {