Skip to content

Commit 193ef4f

Browse files
author
Luca Forstner
committed
Merge branch 'lforst-mimic-native-behaviour-onuncaught-exception' into lforst-fix-nextjs-quitting-on-aborted-requests
2 parents f405686 + 71bcfe0 commit 193ef4f

File tree

4 files changed

+57
-49
lines changed

4 files changed

+57
-49
lines changed
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ Sentry.init({
66
return integrations.map(integration => {
77
if (integration.name === 'OnUncaughtException') {
88
return new Sentry.Integrations.OnUncaughtException({
9-
mimicNativeBehaviour: true,
9+
exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false,
1010
});
1111
} else {
1212
return integration;
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ Sentry.init({
66
return integrations.map(integration => {
77
if (integration.name === 'OnUncaughtException') {
88
return new Sentry.Integrations.OnUncaughtException({
9-
mimicNativeBehaviour: true,
9+
exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false,
1010
});
1111
} else {
1212
return integration;

packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ describe('OnUncaughtException integration', () => {
2828
});
2929
});
3030

31-
describe('with `mimicNativeBehaviour` set to true', () => {
31+
describe('with `exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered` set to false', () => {
3232
test('should close process on uncaught error with no additional listeners registered', done => {
3333
expect.assertions(3);
3434

35-
const testScriptPath = path.resolve(__dirname, 'mimic-behaviour-no-additional-listener-test-script.js');
35+
const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-no-additional-listener-test-script.js');
3636

3737
childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => {
3838
expect(err).not.toBeNull();
@@ -45,7 +45,7 @@ describe('OnUncaughtException integration', () => {
4545
test('should not close process on uncaught error when additional listeners are registered', done => {
4646
expect.assertions(2);
4747

48-
const testScriptPath = path.resolve(__dirname, 'mimic-behaviour-additional-listener-test-script.js');
48+
const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-additional-listener-test-script.js');
4949

5050
childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => {
5151
expect(err).toBeNull();

packages/node/src/integrations/onuncaughtexception.ts

Lines changed: 52 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,11 @@ interface OnUncaughtExceptionOptions {
1313
// Also, we can evaluate using https://nodejs.org/api/process.html#event-uncaughtexceptionmonitor per default, and
1414
// falling back to current behaviour when that's not available.
1515
/**
16-
* Whether the SDK should mimic native behaviour when a global error occurs. Default: `false`
16+
* Whether the SDK should mimic native behaviour when a global error occurs. Default: `true`
1717
* - `false`: The SDK will exit the process on all uncaught errors.
1818
* - `true`: The SDK will only exit the process when there are no other 'uncaughtException' handlers attached.
1919
*/
20-
mimicNativeBehaviour: boolean;
20+
exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: boolean;
2121

2222
/**
2323
* This is called when an uncaught error would cause the process to exit.
@@ -55,7 +55,7 @@ export class OnUncaughtException implements Integration {
5555
*/
5656
public constructor(options: Partial<OnUncaughtExceptionOptions> = {}) {
5757
this._options = {
58-
mimicNativeBehaviour: false,
58+
exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: true,
5959
...options,
6060
};
6161
}
@@ -64,7 +64,7 @@ export class OnUncaughtException implements Integration {
6464
* @inheritDoc
6565
*/
6666
public setupOnce(): void {
67-
global.process.on('uncaughtException', this.handler.bind(this));
67+
global.process.on('uncaughtException', this.handler);
6868
}
6969

7070
/**
@@ -81,6 +81,14 @@ export class OnUncaughtException implements Integration {
8181
let onFatalError: OnFatalErrorHandler = logAndExitProcess;
8282
const client = getCurrentHub().getClient<NodeClient>();
8383

84+
if (this._options.onFatalError) {
85+
// eslint-disable-next-line @typescript-eslint/unbound-method
86+
onFatalError = this._options.onFatalError;
87+
} else if (client && client.getOptions().onFatalError) {
88+
// eslint-disable-next-line @typescript-eslint/unbound-method
89+
onFatalError = client.getOptions().onFatalError as OnFatalErrorHandler;
90+
}
91+
8492
// Attaching a listener to `uncaughtException` will prevent the node process from exiting. We generally do not
8593
// want to alter this behaviour so we check for other listeners that users may have attached themselves and adjust
8694
// exit behaviour of the SDK accordingly:
@@ -99,15 +107,9 @@ export class OnUncaughtException implements Integration {
99107
}
100108
}, 0);
101109

102-
const shouldExitProcess: boolean = !this._options.mimicNativeBehaviour || userProvidedListenersCount === 0;
103-
104-
if (this._options.onFatalError) {
105-
// eslint-disable-next-line @typescript-eslint/unbound-method
106-
onFatalError = this._options.onFatalError;
107-
} else if (client && client.getOptions().onFatalError) {
108-
// eslint-disable-next-line @typescript-eslint/unbound-method
109-
onFatalError = client.getOptions().onFatalError as OnFatalErrorHandler;
110-
}
110+
const processWouldExit = userProvidedListenersCount === 0;
111+
const shouldApplyFatalHandlingLogic =
112+
this._options.exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered || processWouldExit;
111113

112114
if (!caughtFirstError) {
113115
const hub = getCurrentHub();
@@ -125,47 +127,53 @@ export class OnUncaughtException implements Integration {
125127
originalException: error,
126128
data: { mechanism: { handled: false, type: 'onuncaughtexception' } },
127129
});
128-
if (!calledFatalError && shouldExitProcess) {
130+
if (!calledFatalError && shouldApplyFatalHandlingLogic) {
129131
calledFatalError = true;
130132
onFatalError(error);
131133
}
132134
});
133135
} else {
134-
if (!calledFatalError && shouldExitProcess) {
136+
if (!calledFatalError && shouldApplyFatalHandlingLogic) {
135137
calledFatalError = true;
136138
onFatalError(error);
137139
}
138140
}
139-
} else if (calledFatalError && shouldExitProcess) {
140-
// we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down
141-
__DEBUG_BUILD__ &&
142-
logger.warn('uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown');
143-
logAndExitProcess(error);
144-
} else if (!caughtSecondError && shouldExitProcess) {
145-
// two cases for how we can hit this branch:
146-
// - capturing of first error blew up and we just caught the exception from that
147-
// - quit trying to capture, proceed with shutdown
148-
// - a second independent error happened while waiting for first error to capture
149-
// - want to avoid causing premature shutdown before first error capture finishes
150-
// it's hard to immediately tell case 1 from case 2 without doing some fancy/questionable domain stuff
151-
// so let's instead just delay a bit before we proceed with our action here
152-
// in case 1, we just wait a bit unnecessarily but ultimately do the same thing
153-
// in case 2, the delay hopefully made us wait long enough for the capture to finish
154-
// two potential nonideal outcomes:
155-
// nonideal case 1: capturing fails fast, we sit around for a few seconds unnecessarily before proceeding correctly by calling onFatalError
156-
// 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
157-
// note that after hitting this branch, we might catch more errors where (caughtSecondError && !calledFatalError)
158-
// we ignore them - they don't matter to us, we're just waiting for the second error timeout to finish
159-
caughtSecondError = true;
160-
setTimeout(() => {
161-
if (!calledFatalError) {
162-
// it was probably case 1, let's treat err as the sendErr and call onFatalError
163-
calledFatalError = true;
164-
onFatalError(firstError, error);
165-
} else {
166-
// it was probably case 2, our first error finished capturing while we waited, cool, do nothing
141+
} else {
142+
if (shouldApplyFatalHandlingLogic) {
143+
if (calledFatalError) {
144+
// we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down
145+
__DEBUG_BUILD__ &&
146+
logger.warn(
147+
'uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown',
148+
);
149+
logAndExitProcess(error);
150+
} else if (!caughtSecondError) {
151+
// two cases for how we can hit this branch:
152+
// - capturing of first error blew up and we just caught the exception from that
153+
// - quit trying to capture, proceed with shutdown
154+
// - a second independent error happened while waiting for first error to capture
155+
// - want to avoid causing premature shutdown before first error capture finishes
156+
// it's hard to immediately tell case 1 from case 2 without doing some fancy/questionable domain stuff
157+
// so let's instead just delay a bit before we proceed with our action here
158+
// in case 1, we just wait a bit unnecessarily but ultimately do the same thing
159+
// in case 2, the delay hopefully made us wait long enough for the capture to finish
160+
// two potential nonideal outcomes:
161+
// nonideal case 1: capturing fails fast, we sit around for a few seconds unnecessarily before proceeding correctly by calling onFatalError
162+
// 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
163+
// note that after hitting this branch, we might catch more errors where (caughtSecondError && !calledFatalError)
164+
// we ignore them - they don't matter to us, we're just waiting for the second error timeout to finish
165+
caughtSecondError = true;
166+
setTimeout(() => {
167+
if (!calledFatalError) {
168+
// it was probably case 1, let's treat err as the sendErr and call onFatalError
169+
calledFatalError = true;
170+
onFatalError(firstError, error);
171+
} else {
172+
// it was probably case 2, our first error finished capturing while we waited, cool, do nothing
173+
}
174+
}, timeout); // capturing could take at least sendTimeout to fail, plus an arbitrary second for how long it takes to collect surrounding source etc
167175
}
168-
}, timeout); // capturing could take at least sendTimeout to fail, plus an arbitrary second for how long it takes to collect surrounding source etc
176+
}
169177
}
170178
};
171179
}

0 commit comments

Comments
 (0)