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

Can't prevent Sentry SDK from exiting the process even with onFatalError #1661

Closed
Tracked by #9508
crunchtime-ali opened this issue Oct 17, 2018 · 22 comments · Fixed by getsentry/sentry-docs#2643
Closed
Tracked by #9508
Labels
Meta: Help Wanted Package: node Issues related to the Sentry Node SDK
Milestone

Comments

@crunchtime-ali
Copy link

crunchtime-ali commented Oct 17, 2018

@sentry/node version: 4.1.1
Node.js version: 10.9.0

Sentry exits my process after an uncaught exception even I provide my own implementation of onFatalError. How can I prevent that?

Here is a minimal working example. Without sentry.init I get to the 2nd throw. With it the process finisches after the 1st one.

const sentry = require('@sentry/node');

sentry.init(
  { dsn: 'https://key@mysentry.mycompany/2', 
    integrations: [new sentry.Integrations.OnUncaughtException({
      onFatalError: error => {
        console.log('an error occured', error);
      }
    })]
  }
);

process.on('uncaughtException', err => {
  console.log(err, 'Uncaught Exception thrown');
  // process.exit(1);
});

console.log("before throwing the first time")
setTimeout(() => { throw 42 }, 2000)
setTimeout(() => { console.log('still running') }, 10000)
throw 42;
@HazAT
Copy link
Member

HazAT commented Oct 17, 2018

@dj-hedgehog Does this help?
https://docs.sentry.io/learn/draining/?platform=node

@kamilogorek
Copy link
Contributor

kamilogorek commented Oct 17, 2018

onFatalError is only meant to perform a cleanup before we exit the process, not fully prevent it. This is how Node.js is behaving by default (always crash the process and let it restart).

If you want to have a complete control over it, you have to turn off this integration and add handler yourself.

Sentry.init({
  dsn: 'https://key@mysentry.mycompany/2',
  integrations(integrations) {
    return integrations.filter(integration => integration.id !== 'OnUncaughtException')
  }
});

global.process.on('uncaughtException', (error) => {
  const hub = Sentry.getCurrentHub();
  hub.withScope(async (scope) => {
    scope.setLevel('fatal');
    hub.captureException(error, { originalException: error });
  });
});

@crunchtime-ali
Copy link
Author

@kamilogorek Thank you for your quick response. The following code exits the app after the first throw nonetheless:

const sentry = require('@sentry/node');

sentry.init(
  { dsn: 'https://key@mysentry.mycompany/2', 
    integrations(integrations) {
      return integrations.filter(integration => integration.id !== 'OnUncaughtException');
    }
  }
);

global.process.on('uncaughtException', (error) => {
  const hub = sentry.getCurrentHub();
  hub.withScope(async (scope) => {
    scope.setLevel('fatal');
    hub.captureException(error, { originalException: error });
  });
});

console.log("before throwing the first time");
setTimeout(() => { throw "one"}, 1000)
setTimeout(() => { throw "two" }, 2000);
setTimeout(() => { throw "three" }, 4000);
setTimeout(() => { console.log('still running') }, 10000);

@kamilogorek
Copy link
Contributor

@dj-hedgehog it should be integration.name !== 'OnUncaughtException' not integration.id !== 'OnUncaughtException'. Second format is still unreleased, sorry about that 😅

@crunchtime-ali
Copy link
Author

That solved the issue, thanks a lot :)

@JamesHagerman
Copy link

onFatalError is only meant to perform a cleanup before we exit the process, not fully prevent it. This is how Node.js is behaving by default (always crash the process and let it restart).

I just ran into this. It would be wonderful to document the intent spelled out by @kamilogorek somewhere.

Maybe here?: https://docs.sentry.io/platforms/node/configuration/integrations/default-integrations/#onuncaughtexception

@mayfield
Copy link

I hate to necrobump this 2018 issue, but I just ran into this in 2022 and it was very surprising to have my applications behavior changed by the use of Sentry like this. It also cost me several hours of hair pulling out trying to figure out what was happening.

I have always thought one of the design goals of Sentry is to have no meaningful side effects.

Any maintainers care to comment?

@kamilogorek
Copy link
Contributor

kamilogorek commented Mar 15, 2022

What was exactly your case? We are following all the defaults from node's implementation written down in the docs - https://nodejs.org/api/process.html#event-uncaughtexception
We also mention what we do there in our own docs - https://docs.sentry.io/platforms/node/configuration/integrations/default-integrations/#onuncaughtexception

We also had no reports of this being not an intended behavior in almost 4 years, thus my question here.

cc @getsentry/team-web-backend

@mayfield
Copy link

Hi @kamilogorek,

This is in the context of an electron app which also registers and handles uncaughtException with a dialog. Meaning node doesn't kill the process.

I think maybe sentry is assuming that there is no other listener and doing what node would do in that case, ie. exit.

Just a thought, but with node 13+ they have uncaughtExceptionMonitor

https://nodejs.org/api/process.html#event-uncaughtexceptionmonitor

Works for promises and normal exceptions and doesn't have side effects. Almost like, how it should have been in the first place! 😄

@kamilogorek
Copy link
Contributor

We do that for browser SDK, not sure why we decide to skip it here tbh. Might be just something that we missed -

let _oldOnErrorHandler: OnErrorEventHandler = null;
/** JSDoc */
function instrumentError(): void {
_oldOnErrorHandler = global.onerror;
global.onerror = function (msg: any, url: any, line: any, column: any, error: any): boolean {
triggerHandlers('error', {
column,
error,
line,
msg,
url,
});
if (_oldOnErrorHandler) {
// eslint-disable-next-line prefer-rest-params
return _oldOnErrorHandler.apply(this, arguments);
}
return false;
};
}
let _oldOnUnhandledRejectionHandler: ((e: any) => void) | null = null;
/** JSDoc */
function instrumentUnhandledRejection(): void {
_oldOnUnhandledRejectionHandler = global.onunhandledrejection;
global.onunhandledrejection = function (e: any): boolean {
triggerHandlers('unhandledrejection', e);
if (_oldOnUnhandledRejectionHandler) {
// eslint-disable-next-line prefer-rest-params
return _oldOnUnhandledRejectionHandler.apply(this, arguments);
}
return true;
};
}

TIL about uncaughtExceptionMonitor, thanks! We still do need to support older versions, so we'd need to do something similar to browser implementation instead.
I wonder if it can handle async code though, which we need to flush remaining events from the buffer -


There's a possibility that monitor will trigger events, and let it fall through to uncaughtException handler, which will in turns kill the process before letting requests out.

I'll notify the team to backlog this issue :)

@ibc
Copy link

ibc commented May 25, 2022

Our Node app sets a process.on('uncaughtException') listener that avoids process termination when an uncaught error happens. Then we enable Sentry and Sentry decides that the process should terminate on uncaught exception ¯_(ツ)_/¯

TIL about uncaughtExceptionMonitor, thanks! We still do need to support older versions, so we'd need to do something similar to browser implementation instead.

Within the uncaughtException listener that Sentry installs, it could count the number of event listeners for uncaughtException and if >1 then do NOT exit the process.

@AbhiPrasad
Copy link
Member

Within the uncaughtException listener that Sentry installs, it could count the number of event listeners for uncaughtException and if >1 then do NOT exit the process.

We could also expose an option here for full backwards compatibility. Re-opening and adding to the backlog. PRs welcome!

@AbhiPrasad AbhiPrasad reopened this May 30, 2022
@AbhiPrasad AbhiPrasad added Meta: Help Wanted Package: node Issues related to the Sentry Node SDK Status: Backlog labels May 30, 2022
@lforst
Copy link
Member

lforst commented May 30, 2022

Not sure if that would work but we could also check for any listeners using process.listeners('uncaughtException') and adjust the SDK's behaviour based on that. (i.e. exit process if no other listeners, don't exit if there are other handlers)

@ibc
Copy link

ibc commented May 30, 2022

PR here #5176

lforst pushed a commit to ibc/sentry-javascript that referenced this issue May 30, 2022
@lforst lforst added this to the 8.0.0 milestone Jun 1, 2022
@dzlandis
Copy link

dzlandis commented Aug 8, 2022

Our Node app sets a process.on('uncaughtException') listener that avoids process termination when an uncaught error happens. Then we enable Sentry and Sentry decides that the process should terminate on uncaught exception ¯_(ツ)_/¯

Just ran into this same problem. It is extremely annoying and frustrating! I had to do a lot of digging just to find this issue regarding it. Please fix this 🙏🏻

@maxpain
Copy link

maxpain commented Oct 29, 2022

The same problem

@HazAT
Copy link
Member

HazAT commented Jan 26, 2023

Related: #4154

@HazAT
Copy link
Member

HazAT commented Feb 16, 2023

Related: #6146

@chr4ss12
Copy link

chr4ss12 commented Apr 4, 2023

same problem here, as it stands currently sentry is not usable for us

@AbhiPrasad
Copy link
Member

As per the Node docs if an unhandled rejection is triggered the process should be exited: https://nodejs.org/api/process.html#warning-using-uncaughtexception-correctly.

The correct use of 'uncaughtException' is to perform synchronous cleanup of allocated resources (e.g. file descriptors, handles, etc) before shutting down the process. It is not safe to resume normal operation after 'uncaughtException'.

To restart a crashed application in a more reliable way, whether 'uncaughtException' is emitted or not, an external monitor should be employed in a separate process to detect application failures and recover or restart as needed.

If you want to override Node's recommendation, you can override this via the exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered option.

Sentry.init({
  dsn: 'https://public@dsn.ingest.sentry.io/1337',
  integrations: integrations => {
    return integrations.map(integration => {
      if (integration.name === 'OnUncaughtException') {
        // override `OnUncaughtException` integration to not exit.
        return new Sentry.Integrations.OnUncaughtException({
          // do not exit if other uncaught exception handlers are registered.
          exitEvenIfOtherHandlersAreRegistered: false,
        });
      } else {
        return integration;
      }
    });
  },
});

In a future major version we will make this API better. Thanks!

@lforst
Copy link
Member

lforst commented Apr 10, 2024

I tried getting uncaughtExceptionMonitor to work but it basically boils down to what elastic/apm-agent-nodejs#2367 (comment) outlines perfectly:

  • When an exception happens, Node.js will not wait for any requests kicked off in the uncaughtExceptionMonitor to finish, meaning our requests will not be flushed.
  • The only way around this is spawning a sync child process that is tasked with sending the event. This means however that any async event processors (like source code context lines) will not run.

Since this means a lot of effort for arguably little gain (and potentially many more pitfalls), I think we will stick with the current approach for now.

@lforst
Copy link
Member

lforst commented Apr 10, 2024

I am gonna mark this as resolved with #11532. This will be shipped as a breaking change as part of the upcoming major v8 release.

Please see the migration docs here: https://github.com/getsentry/sentry-javascript/blob/develop/MIGRATION.md#behaviour-in-combination-with-onuncaughtexception-handlers-in-nodejs

As always, feel free to ping us here for any questions or concerns!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment