Skip to content

Commit 71bcfe0

Browse files
author
Luca Forstner
committed
cleanup
1 parent 8922b5b commit 71bcfe0

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
@@ -12,11 +12,11 @@ interface OnUncaughtExceptionOptions {
1212
// Also, we can evaluate using https://nodejs.org/api/process.html#event-uncaughtexceptionmonitor per default, and
1313
// falling back to current behaviour when that's not available.
1414
/**
15-
* Whether the SDK should mimic native behaviour when a global error occurs. Default: `false`
15+
* Whether the SDK should mimic native behaviour when a global error occurs. Default: `true`
1616
* - `false`: The SDK will exit the process on all uncaught errors.
1717
* - `true`: The SDK will only exit the process when there are no other 'uncaughtException' handlers attached.
1818
*/
19-
mimicNativeBehaviour: boolean;
19+
exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: boolean;
2020

2121
/**
2222
* This is called when an uncaught error would cause the process to exit.
@@ -53,7 +53,7 @@ export class OnUncaughtException implements Integration {
5353
*/
5454
public constructor(options: Partial<OnUncaughtExceptionOptions> = {}) {
5555
this._options = {
56-
mimicNativeBehaviour: false,
56+
exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: true,
5757
...options,
5858
};
5959
}
@@ -62,7 +62,7 @@ export class OnUncaughtException implements Integration {
6262
* @inheritDoc
6363
*/
6464
public setupOnce(): void {
65-
global.process.on('uncaughtException', this.handler.bind(this));
65+
global.process.on('uncaughtException', this.handler);
6666
}
6767

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

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

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

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

0 commit comments

Comments
 (0)