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.

Sign up for GitHub

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

Jump to bottom

fix(nextjs): Do not exit process when errors bubble up while additional uncaughtException-handlers are registered #6138

Merged
merged 10 commits into from Nov 4, 2022
Merged

fix(nextjs): Do not exit process when errors bubble up while additional uncaughtException-handlers are registered #6138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8922b5b
feat(node): Add option to `OnUncaughtException` integration that allo…
lforst Nov 4, 2022
55ed724
fix(nextjs): Do not exit process when errors bubble up while addition…
lforst Nov 4, 2022
71bcfe0
cleanup
lforst Nov 4, 2022
f405686
Change option name
lforst Nov 4, 2022
193ef4f
Merge branch 'lforst-mimic-native-behaviour-onuncaught-exception' int…
lforst Nov 4, 2022
74ab0bd
Rename option
lforst Nov 4, 2022
05bfb5e
Rephrase JSDoc
lforst Nov 4, 2022
2ddefff
Merge remote-tracking branch 'origin/lforst-mimic-native-behaviour-on…
lforst Nov 4, 2022
85faa2a
Rename option
lforst Nov 4, 2022
0157588
Merge branch 'master' into lforst-fix-nextjs-quitting-on-aborted-requ…
lforst Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/nextjs/src/index.server.ts
View file
Open in desktop
Expand Up @@ -118,6 +118,11 @@ function addServerIntegrations(options: NextjsOptions): void {
});
integrations = addOrUpdateIntegration(defaultRewriteFramesIntegration, integrations);

const nativeBehaviourOnUncaughtException = new Integrations.OnUncaughtException();
integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, {
_options: { exitEvenIfOtherHandlersAreRegistered: false },
});
Comment on lines +121 to +124
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by this. The point of addOrUpdateIntegration is to say, "here's the integration to add if no instance is already there, but if an instance is there, update it with these options." This means that the integration to add should have the same options as the the ones which are being updated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I assumed the options didn't matter since the UncaughtException integration is part of the default integrations anyways but you're right - we should add the options in case the users override the integration.


if (hasTracingEnabled(options)) {
const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });
integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {
Expand Down
2 changes: 2 additions & 0 deletions packages/node/src/integrations/onuncaughtexception.ts
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -48,6 +49,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;

/**
Expand Down