Repository: microsoft/ApplicationInsights-node.js (ts)

Issue:
[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.

<img width="992" alt="Screen Shot 2020-01-20 at 1 56 42 PM" src="https://user-images.githubusercontent.com/369698/72753496-16626580-3b93-11ea-8f22-025b5940c659.png">

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:

<img width="1032" alt="Screen Shot 2020-01-20 at 2 45 57 PM" src="https://user-images.githubusercontent.com/369698/72753723-adc7b880-3b93-11ea-9c03-3ae8dfca06ef.png">

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:

<img width="1100" alt="Screen Shot 2020-01-20 at 1 55 41 PM" src="https://user-images.githubusercontent.com/369698/72753626-64776900-3b93-11ea-9469-440c8389c150.png">

**Steps to Reproduce**
1. (with app insights active) using latest redis package connect to a server with a retry strategy defined for the client
2. 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.


Pull request description:
fix: skip rethrow when other handlers exist
Fixes #584 
Handle-time check to suppress rethrows when other handles exist. Alternative would be to expose 2nd `boolean | Function` arg to `setAutoCollectionExceptions(true, rethrowHandler)`. 

Node<13.7 Behavior
- If multiple uncaught handlers exist: no crash
- If this SDK is the only handler: crash (status=1)

Node13.7+ behavior
- For node >=13.7, use `uncaughtExceptionMonitor` instead. Process will only crash (default status code) if no `uncaughtException` handlers exist.

/cc @avoidwork - Please let me know your reasoning if you prefer explicitly providing an exception handler!

Patch:
diff --git a/AutoCollection/Exceptions.ts b/AutoCollection/Exceptions.ts
index 37998db..30ce41f 100644
--- a/AutoCollection/Exceptions.ts
+++ b/AutoCollection/Exceptions.ts
@@ -1,6 +1,7 @@
 import http = require("http");
 
 import Contracts = require("../Declarations/Contracts");
+import Logging = require("../Library/Logging");
 import TelemetryClient = require("../Library/TelemetryClient");
 import Sender = require("../Library/Sender");
 import Queue = require("../Library/Channel");
@@ -9,10 +10,13 @@ import Util = require("../Library/Util");
 class AutoCollectExceptions {
 
     public static INSTANCE: AutoCollectExceptions = null;
-    public static get UNCAUGHT_EXCEPTION_HANDLER_NAME(): string { return "uncaughtException"; }
-    public static get UNHANDLED_REJECTION_HANDLER_NAME(): string { return "unhandledRejection"; }
+    public static UNCAUGHT_EXCEPTION_MONITOR_HANDLER_NAME = "uncaughtExceptionMonitor";
+    public static UNCAUGHT_EXCEPTION_HANDLER_NAME = "uncaughtException";
+    public static UNHANDLED_REJECTION_HANDLER_NAME = "unhandledRejection";
 
+    private static _RETHROW_EXIT_MESSAGE = "Application Insights Rethrow Exception Handler";
     private static _FALLBACK_ERROR_MESSAGE = "A promise was rejected without providing an error. Application Insights generated this error stack for you.";
+    private static _canUseUncaughtExceptionMonitor = false;
     private _exceptionListenerHandle: (reThrow: boolean, error: Error) => void;
     private _rejectionListenerHandle: (reThrow: boolean, error: Error) => void;
     private _client: TelemetryClient;
@@ -25,6 +29,10 @@ class AutoCollectExceptions {
 
         AutoCollectExceptions.INSTANCE = this;
         this._client = client;
+
+        // Only use for 13.7.0+
+        const nodeVer = process.versions.node.split(".");
+        AutoCollectExceptions._canUseUncaughtExceptionMonitor = parseInt(nodeVer[0]) > 13 || (parseInt(nodeVer[0]) === 13 && parseInt(nodeVer[1]) >= 7);
     }
 
     public isInitialized() {
@@ -38,25 +46,36 @@ class AutoCollectExceptions {
             if (!this._exceptionListenerHandle) {
                 // For scenarios like Promise.reject(), an error won't be passed to the handle. Create a placeholder
                 // error for these scenarios.
-                var handle = (reThrow: boolean, error: Error = new Error(AutoCollectExceptions._FALLBACK_ERROR_MESSAGE)) => {
+                var handle = (reThrow: boolean, name?: string, error: Error = new Error(AutoCollectExceptions._FALLBACK_ERROR_MESSAGE)) => {
                     this._client.trackException({ exception: error });
                     this._client.flush({ isAppCrashing: true });
-                    if (reThrow) {
-                        var THIS_IS_APPLICATION_INSIGHTS_RETHROWING_YOUR_EXCEPTION = error;
-                        throw THIS_IS_APPLICATION_INSIGHTS_RETHROWING_YOUR_EXCEPTION; // Error originated somewhere else in your app
+                    // only rethrow when we are the only listener
+                    if (reThrow && name && process.listeners(name).length === 1) {
+                        Logging.warn(AutoCollectExceptions._RETHROW_EXIT_MESSAGE, name, error);
+                        process.exit(1);
                     }
                 };
-                this._exceptionListenerHandle = handle.bind(this, true);
-                this._rejectionListenerHandle = handle.bind(this, false);
 
-                process.on(AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME, this._exceptionListenerHandle);
-                process.on(AutoCollectExceptions.UNHANDLED_REJECTION_HANDLER_NAME, this._rejectionListenerHandle);
+                if (AutoCollectExceptions._canUseUncaughtExceptionMonitor) {
+                    // Node.js >= 13.7.0, use uncaughtExceptionMonitor. It handles both promises and exceptions
+                    this._exceptionListenerHandle = handle.bind(this, false); // never rethrows
+                    process.on(AutoCollectExceptions.UNCAUGHT_EXCEPTION_MONITOR_HANDLER_NAME, this._exceptionListenerHandle);
+                } else {
+                    this._exceptionListenerHandle = handle.bind(this, true, AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME);
+                    this._rejectionListenerHandle = handle.bind(this, false); // never rethrows
+                    process.on(AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME, this._exceptionListenerHandle);
+                    process.on(AutoCollectExceptions.UNHANDLED_REJECTION_HANDLER_NAME, this._rejectionListenerHandle);
+                }
             }
 
         } else {
             if (this._exceptionListenerHandle) {
-                process.removeListener(AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME, this._exceptionListenerHandle);
-                process.removeListener(AutoCollectExceptions.UNHANDLED_REJECTION_HANDLER_NAME, this._rejectionListenerHandle);
+                if (AutoCollectExceptions._canUseUncaughtExceptionMonitor) {
+                    process.removeListener(AutoCollectExceptions.UNCAUGHT_EXCEPTION_MONITOR_HANDLER_NAME, this._exceptionListenerHandle);
+                } else {
+                    process.removeListener(AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME, this._exceptionListenerHandle);
+                    process.removeListener(AutoCollectExceptions.UNHANDLED_REJECTION_HANDLER_NAME, this._rejectionListenerHandle);
+                }
                 this._exceptionListenerHandle = undefined;
                 this._rejectionListenerHandle = undefined;
                 delete this._exceptionListenerHandle;


Test patch:
diff --git a/Tests/AutoCollection/Exceptions.tests.ts b/Tests/AutoCollection/Exceptions.tests.ts
index c464451..83de8fc 100644
--- a/Tests/AutoCollection/Exceptions.tests.ts
+++ b/Tests/AutoCollection/Exceptions.tests.ts
@@ -11,14 +11,27 @@ describe("AutoCollection/Exceptions", () => {
             AppInsights.dispose();
         });
 
+        it("should use uncaughtExceptionMonitor for node 13.7.0+", () => {
+            var nodeVer = process.versions.node.split(".");
+            var expectation = parseInt(nodeVer[0]) > 13 || (parseInt(nodeVer[0]) === 13 && parseInt(nodeVer[1]) >= 7);
+            var exceptions = new AutoCollectionExceptions(null);
+            assert.equal(AutoCollectionExceptions["_canUseUncaughtExceptionMonitor"], expectation);
+        });
+
         it("disables autocollection", () => {
             var processOnSpy = sinon.spy(global.process, "on");
             var processRemoveListenerSpy = sinon.spy(global.process, "removeListener");
 
             AppInsights.setup("key").setAutoCollectExceptions(true).start();
-            assert.equal(processOnSpy.callCount, 2, "After enabling exception autocollection, there should be 2 calls to processOnSpy");
-            assert.equal(processOnSpy.getCall(0).args[0], AutoCollectionExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME);
-            assert.equal(processOnSpy.getCall(1).args[0], AutoCollectionExceptions.UNHANDLED_REJECTION_HANDLER_NAME);
+
+            if (AutoCollectionExceptions["_canUseUncaughtExceptionMonitor"]) {
+                assert.equal(processOnSpy.callCount, 1, "After enabling exception autocollection, there should be 1 call to processOnSpy");
+                assert.equal(processOnSpy.getCall(0).args[0], AutoCollectionExceptions.UNCAUGHT_EXCEPTION_MONITOR_HANDLER_NAME);
+            } else {
+                assert.equal(processOnSpy.callCount, 2, "After enabling exception autocollection, there should be 2 calls to processOnSpy");
+                assert.equal(processOnSpy.getCall(0).args[0], AutoCollectionExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME);
+                assert.equal(processOnSpy.getCall(1).args[0], AutoCollectionExceptions.UNHANDLED_REJECTION_HANDLER_NAME);
+            }
             processOnSpy.restore();
             processRemoveListenerSpy.restore();
         });
diff --git a/Tests/js/endToEnd.js b/Tests/js/endToEnd.js
index d0abd15..2224bda 100644
--- a/Tests/js/endToEnd.js
+++ b/Tests/js/endToEnd.js
@@ -1,4 +1,5 @@
 const assert = require('assert');
+const sinon = require('sinon');
 
 // Special embedded test cases for testing if app can close
 if (process.argv.indexOf('embeddedTestCase-AppTerminates1') > -1) {
@@ -68,4 +69,69 @@ describe('module', function () {
             appInsights.dispose();
         });
     });
+
+    describe('uncaught exceptions', function() {
+        var UNCAUGHT_EXCEPTION = 'uncaughtException';
+        var UNCAUGHT_EXCEPTION_MONITOR = 'uncaughtExceptionMonitor';
+        var exitStub;
+        var mochaListener;
+
+        var getLegacyHandler = function() {
+            return process.listeners(UNCAUGHT_EXCEPTION)[0];
+        }
+
+        var getMonitorHandler = function() {
+            return process.listeners(UNCAUGHT_EXCEPTION_MONITOR)[0];
+        }
+
+        before(function() {
+            mochaListener = process.listeners(UNCAUGHT_EXCEPTION).pop();
+            process.removeListener(UNCAUGHT_EXCEPTION, mochaListener);
+            exitStub = sinon.stub(process, 'exit');
+        });
+
+        beforeEach(function() {
+            exitStub.reset();
+        });
+
+        after(function() {
+            process.addListener(UNCAUGHT_EXCEPTION, mochaListener);
+            exitStub.restore();
+        });
+
+        it('should crash on uncaught exceptions', function () {
+            var appInsights = require('../../');
+            appInsights.setup('ikey').setAutoCollectExceptions(true).start();
+            assert.ok(appInsights.defaultClient);
+            var handler;
+            if (handler = getLegacyHandler()) {
+                handler(new Error('legacy error'));
+                assert.equal(exitStub.callCount, 1, 'Legacy handler rethrows when no');
+            } else if (handler = getMonitorHandler()) {
+                handler(new Error('monitor handler'));
+                assert.equal(exitStub.callCount, 0, 'Monitor handler does not cause the throw');
+            } else {
+                assert.ok(false, 'No handler found');
+            }
+            appInsights.defaultClient.flush();
+            appInsights.dispose();
+        });
+
+        it('should not crash on uncaught exceptions if multiple handlers exist', function () {
+            var appInsights = require('../../');
+            appInsights.setup('ikey').setAutoCollectExceptions(true).start();
+            process.addListener(UNCAUGHT_EXCEPTION, function() {});
+            assert.ok(appInsights.defaultClient);
+
+            assert.doesNotThrow(function() {
+                var handler = getMonitorHandler() || getLegacyHandler();
+                handler(new Error('existing handler error'));
+                assert.equal(exitStub.callCount, 0, 'Handler does not rethrow when other handles exist');
+            });
+
+            appInsights.defaultClient.flush();
+            appInsights.dispose();
+            process.removeAllListeners(UNCAUGHT_EXCEPTION);
+        });
+    });
 });