bugsnag / bugsnag-node

[DEPRECATED] Please upgrade to our Universal JS notifier "@bugsnag/js" • https://github.com/bugsnag/bugsnag-js
https://www.bugsnag.com/
MIT License
48 stars 55 forks source link

Bugsnag: Encountered an uncaught error, terminating. All uncaught errors force process.exit(), bad. #154

Closed ian closed 5 years ago

ian commented 6 years ago

Expected behavior

The error is handled by the underlying framework / server. If the server considers the error an exit event, it shuts down the process. If the error is handled by the framework then it makes the call on shutting down the process.

Observed behavior

Bugsnag force exits the process

Steps to reproduce

Occurrs here: https://github.com/bugsnag/bugsnag-node/blob/151a789f27742f345324269607c1cc3780da4d8c/lib/configuration.js#L44

Version

Any

Additional information

I have no idea why this is behavior built into Bugsnag. It's quite infuriating.

Bugsnag shouldn't change how an underlying application performs. If the server implementation / framework wants to exit, let it handle on it's own.

The default behavior for uncaught errors should be to add nothing to the underlying implementation. It should NOT force exit the process.

Ideally Bugsnag should console log what happened, and if in development, make a note to the user to override onUncaughtError if special functionality is needed.

bengourley commented 6 years ago

Hey @ian. Bugsnag has to close the process by default when handling an unhandled error to mimic what Node would normally do…

If you don't want the process to exit, bear in mind that this is strongly discourged, as you have no idea what state your system is from now on, but you can (as you spotted) override the onUncaughtError option to do whatever you like.

If there is some situation or error that you've found which wouldn't normally cause a Node process (without Bugsnag) to terminate, but does when you use Bugsnag, let me know.

ian commented 6 years ago

@bengourley If you were mimicking what node would do you would simply let the exception propagate. What you're actually doing is introducing new behavior to the system.

As an example, suppose you buy an OEM reader for your car. You plug it in and it detects an error that it's not sure how to handle. It shuts down your engine as a result. Does this seem like normal behavior?

You're introducing unrequested behavior as a default. Your competitors at least make this an option to exit on unhandled, but this is off by default: https://github.com/rollbar/rollbar.js/blob/3027a29538131dc9d027235757bdd174b2b2eb96/src/server/rollbar.js#L468

FTR love your product, best of the space.

bengourley commented 6 years ago

If you were mimicking what node would do you would simply let the exception propagate. What you're actually doing is introducing new behavior to the system.

That's not true – unhandled exceptions crash the process. Node has default behaviour when there is no uncaughtException handler which never gets called once a custom one is added.

As an example, suppose you buy an OEM reader for your car. You plug it in and it detects an error that it's not sure how to handle. It shuts down your engine as a result. Does this seem like normal behavior?

To use your analogy, the error detected shuts down the engine, but when you observe it you want it to keep the engine running? Even though the manufacturer of the car decided that this error means it should shut down? That's fine – you can do it by customising onUncaughtError but we're not going to do it by default.

All that said – if you show me an example of an error that doesn't crash the process when Bugsnag isn't in use, but does when Bugsnag is in use, that sounds wrong, and we should fix it!

TimNZ commented 5 years ago

Bugsnag is erroneously terminating my node app on an error that must be caught or ignored upstream. If I don't init Bugsnag the app continues to run fine.

Bugsnag:  Encountered an uncaught error, terminating…
Error: Can't statLink "/Users/timshnaider/Desktop/Development/customers/gamefroot/api2/public/gamefroot_cache/static.gamefroot.com/": ENOENT: no such file or directory, lstat '/Users/timshnaider/Desktop/Development/customers/gamefroot/api2/public/gamefroot_cache/static.gamefroot.com/'
    at Error (native)
snmaynard commented 5 years ago

Would you be able to either gist a simple example or explain how you are generating the error so we can test it? Googling for Can't statLink doesnt seem to find this specific error. Is it a native error?

TimNZ commented 5 years ago

Sure will, in the next week. It's being thrown in a nested chain and obviously caught somewhere.

I just posted that example to confirm it is a real world undesirable situation where Bugsnag terminates on an error apparently uncaught globally. And agree with OP that default behaviour of terminating is not ideal.

snmaynard commented 5 years ago

Yeah - it's unfortunate that listening to the uncaughtException event changes the behaviour of nodejs applications. If this particular error doesnt bubble up to that handler though and we still exit then that would be a bug on our side and one we would prioritise fixing quickly.

You can read more about the handler at https://nodejs.org/api/process.html#process_event_uncaughtexception

Ideally adding bugsnag to an application will not change its behaviour at all - and that is what we strive for here.

TimNZ commented 5 years ago

I don't want to spend hours tracing through bugsnag code and ensuring my understanding of error handling in node is solid, so tell me if my understanding is correct.

With no bugsnag the reject by qio which has no catch doesn't propagate as it is has no return (and I think callers are promise aware in this version of Sails), whereas with bugsnag you are handling uncaughtException which gets notified of the uncaught reject?

I didn't write this code, it was done by an amateur dev, just want to understand what is happening.

screen shot 2018-11-15 at 11 43 33 am
snmaynard commented 5 years ago

I just played about a little with the following code to see how unhandledrejections and uncaughtexception events alter node's default behaviour. You can see that currently if you dont listen to uncaughtexception and you throw, it prints a stack and quits. If you listen to the event it doesnt do this however.

With unhandledrejection, if you listen to the event everything works as you'd expect, if you dont listen to the event it prints a warning saying in future this is going to kill the app (DEP0018). This is the case with version 8 and version 10 of node.

var process = require('process');

process.on('unhandledRejection', (reason, promise) => {
  console.log('Unhandled Rejection at:', promise, 'reason:', reason);
})

process.on('uncaughtException', (error) => {
  console.log('Uncaught exception: ', error);
})

throw new Error('something went wrong');

new Promise((resolve, reject) => {
  reject('error');
})

setTimeout(() => {
  console.log('after timeout');
}, 100)

What we really want to do is be able to call the current version of node's default handler and maybe this issue will resolve itself as we can be transparent in that case then. We can investigate to see if we can get a reference to the default event handlers. I think there is an issue right now where if we listen to unhandled rejections we terminate the app - which is something we should fix.

TimNZ commented 5 years ago

Looks solid.

This code is running on v6.11.4, and I only did a quick google to see if unhandledRejection was available in that ver and then gave up without answering it.

As OP asked for, Bugsnag shouldn't change current behaviour unless explicitly told to do so.

bengourley commented 5 years ago

Hey all, we just release our new "universal" JS notifer @bugsnag/js which should now be used for all JavaScript (browser and node). Upgrade guide is here.

This module is now deprecated so I'm closing off this issue. The new notifier adheres to this constraint:

Bugsnag shouldn't change current behaviour unless explicitly told to do so

So please upgrade when you get the chance, and if you find it doing something unexpected, let us know!