Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(node): Do not exit process by default when other onUncaughtException handlers are registered in onUncaughtExceptionIntegration #11532

Merged
merged 2 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,7 @@ Sentry.init({
- [Updated behaviour of `extraErrorDataIntegration`](./MIGRATION.md#extraerrordataintegration-changes)
- [Updated behaviour of `transactionContext` passed to `tracesSampler`](./MIGRATION.md#transactioncontext-no-longer-passed-to-tracessampler)
- [Updated behaviour of `getClient()`](./MIGRATION.md#getclient-always-returns-a-client)
- [Updated behaviour of the SDK in combination with `onUncaughtException` handlers in Node.js](./MIGRATION.md#behaviour-in-combination-with-onuncaughtexception-handlers-in-node.js)
- [Removal of Client-Side health check transaction filters](./MIGRATION.md#removal-of-client-side-health-check-transaction-filters)
- [Change of Replay default options (`unblock` and `unmask`)](./MIGRATION.md#change-of-replay-default-options-unblock-and-unmask)
- [Angular Tracing Decorator renaming](./MIGRATION.md#angular-tracing-decorator-renaming)
Expand Down Expand Up @@ -1168,6 +1169,16 @@ some attributes may only be set later during the span lifecycle (and thus not be
Sentry was actually initialized, using `getClient()` will thus not work anymore. Instead, you should use the new
`Sentry.isInitialized()` utility to check this.

#### Behaviour in combination with `onUncaughtException` handlers in Node.js

Previously the SDK exited the process by default, even though additional `onUncaughtException` may have been registered,
that would have prevented the process from exiting. You could opt out of this behaviour by setting the
`exitEvenIfOtherHandlersAreRegistered: false` in the `onUncaughtExceptionIntegration` options. Up until now the value
for this option defaulted to `true`.

Going forward, the default value for `exitEvenIfOtherHandlersAreRegistered` will be `false`, meaning that the SDK will
not exit your process when you have registered other `onUncaughtException` handlers.

#### Removal of Client-Side health check transaction filters

The SDK no longer filters out health check transactions by default. Instead, they are sent to Sentry but still dropped
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@ describe('OnUncaughtException integration', () => {
});
});

test('should close process on uncaught error when additional listeners are registered', done => {
expect.assertions(3);
test('should not close process on uncaught error when additional listeners are registered', done => {
expect.assertions(2);

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!");
expect(err).toBeNull();
expect(stdout).toBe("I'm alive!");
done();
});
});
Expand Down
4 changes: 0 additions & 4 deletions packages/nextjs/src/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,12 @@ import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolica
import { getVercelEnv } from '../common/getVercelEnv';
import { isBuild } from '../common/utils/isBuild';
import { distDirRewriteFramesIntegration } from './distDirRewriteFramesIntegration';
import { onUncaughtExceptionIntegration } from './onUncaughtExceptionIntegration';

export * from '@sentry/node';
import type { EventProcessor } from '@sentry/types';
import { requestIsolationScopeIntegration } from './requestIsolationScopeIntegration';

export { captureUnderscoreErrorException } from '../common/_error';
export { onUncaughtExceptionIntegration } from './onUncaughtExceptionIntegration';

const globalWithInjectedValues = GLOBAL_OBJ as typeof GLOBAL_OBJ & {
__rewriteFramesDistDir__?: string;
Expand Down Expand Up @@ -75,13 +73,11 @@ export function init(options: NodeOptions): void {
const customDefaultIntegrations = [
...getDefaultIntegrations(options).filter(
integration =>
integration.name !== 'OnUncaughtException' &&
// Next.js comes with its own Node-Fetch instrumentation, so we shouldn't add ours on-top
integration.name !== 'NodeFetch' &&
// Next.js comes with its own Http instrumentation for OTel which lead to double spans for route handler requests
integration.name !== 'Http',
),
onUncaughtExceptionIntegration(),
requestIsolationScopeIntegration(),
];

Expand Down
5 changes: 0 additions & 5 deletions packages/nextjs/src/server/onUncaughtExceptionIntegration.ts

This file was deleted.

25 changes: 9 additions & 16 deletions packages/node/src/integrations/onuncaughtexception.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { captureException, defineIntegration } from '@sentry/core';
import { getClient } from '@sentry/core';
import type { IntegrationFn } from '@sentry/types';
import { logger } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
Expand All @@ -13,17 +12,13 @@ type TaggedListener = NodeJS.UncaughtExceptionListener & {
tag?: string;
};

// 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
// 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`
* Default: `false`
*/
exitEvenIfOtherHandlersAreRegistered: boolean;

Expand All @@ -40,24 +35,22 @@ interface OnUncaughtExceptionOptions {

const INTEGRATION_NAME = 'OnUncaughtException';

const _onUncaughtExceptionIntegration = ((options: Partial<OnUncaughtExceptionOptions> = {}) => {
const _options = {
exitEvenIfOtherHandlersAreRegistered: true,
/**
* Add a global exception handler.
*/
export const onUncaughtExceptionIntegration = defineIntegration((options: Partial<OnUncaughtExceptionOptions> = {}) => {
const optionsWithDefaults = {
exitEvenIfOtherHandlersAreRegistered: false,
...options,
};

return {
name: INTEGRATION_NAME,
setup(client: NodeClient) {
global.process.on('uncaughtException', makeErrorHandler(client, _options));
global.process.on('uncaughtException', makeErrorHandler(client, optionsWithDefaults));
},
};
}) satisfies IntegrationFn;

/**
* Add a global exception handler.
*/
export const onUncaughtExceptionIntegration = defineIntegration(_onUncaughtExceptionIntegration);
});

type ErrorHandler = { _errorHandler: boolean } & ((error: Error) => void);

Expand Down
Loading