ApplicationInsights-node.js: [BUG] AutoCollectExceptions.enable re-throws an uncaught exception into the ether which can crash the process with status 7

Description/Screenshot The redis package retry_strategy handler appears to have a problem with this feature of the applicationinsights package, such that a retry never happens due to this extra throw.

The screenshot above has an event registered for uncaughtException but it doesn’t execute due to the process exiting with status 7; the trigger of the exception is node.js > net.js > client which is utilized for a redis client.

The screenshot below is proper behavior as coded in my app:

The uncaughtException event handler in my code would do some ops, then process.exit(1) to trigger a restart with a revised configuration; because this never runs I had processes restart with the same config in production:

Steps to Reproduce

(with app insights active) using latest redis package connect to a server with a retry strategy defined for the client
stop redis, or disconnect the client uncleanly

OS/Browser:
SDK Version [e.g. 22]:
How you initialized the SDK:

Expected behavior Application error is logged, application continues to run (no exit 7 status)

Additional context Application is utilizing tenso for API server functionality, but that does not handle uncaughtException events.

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 20 (6 by maintainers)

Most upvoted comments

The problem it’s aimed at isn’t real, nor is the ‘to crash or not to crash’ question for app-insights to consider or answer - that’s not it’s role… it reports.

Absolutely agree here. In Node 13.7+, this is simple. I think this SDK should listen to uncaughtExceptionMonitor and be out of this business of managing crash state entirely.

Unfortunately in older versions, the only API we have forces us into this question. We should only report, but the only API available to us to report on unhandled exceptions changes program behavior just by listening to it.

Rethrowing was definitely a bug in all cases - as you pointed out there’s behavior change in the status code. My intent in the sample snippet is to keep the default behavior in the Node documentation maintained:

By default, Node.js handles such exceptions by printing the stack trace to stderr and exiting with code 1, overriding any previously set process.exitCode. Adding a handler for the ‘uncaughtException’ event overrides this default behavior.

https://nodejs.org/api/process.html#process_event_uncaughtexception

So with the technique in my sample snippet, the AI SDK would track the exception, print it out (example is console log but it should go to stderr), and then exit with code 1. If another handler is found, just like Node default, the printing and exit behavior is skipped - allowing the user’s own handler to define behavior.

It’s important that merely using the SDK doesn’t change app behavior - that’s how this bug with rethrows was caught after all! Apps that crash without this SDK should always crash with it. By not having any handling to retain default Node exit behavior, I could easily see consumers using this SDK in non-server scenarios (examples here are CLI scripts and desktop programs like VS Code) ending in really unexpected states.

Would the proposed solution in my sample resolve your use case? I believe it should allow your existing handlers to operate as if this SDK were not even on the box

OsvaldoRosado on Jan 28, 2020