feat(node): Add option to OnUncaughtException integration that allo…

…ws mimicking native uncaught error exit behaviour (#6137)

packages/node-integration-tests/suites/public-api/OnUncaughtException/additional-listener-test-script.js

@@ -0,0 +1,16 @@
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+});
+
+process.on('uncaughtException', () => {
+  // do nothing - this will prevent the Error below from closing this process before the timeout resolves
+});
+
+setTimeout(() => {
+  process.stdout.write("I'm alive!");
+  process.exit(0);
+}, 500);
+
+throw new Error();

packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-additional-listener-test-script.js

@@ -0,0 +1,27 @@
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: integrations => {
+    return integrations.map(integration => {
+      if (integration.name === 'OnUncaughtException') {
+        return new Sentry.Integrations.OnUncaughtException({
+          exitEvenIfOtherHandlersAreRegistered: false,
+        });
+      } else {
+        return integration;
+      }
+    });
+  },
+});
+
+process.on('uncaughtException', () => {
+  // do nothing - this will prevent the Error below from closing this process before the timeout resolves
+});
+
+setTimeout(() => {
+  process.stdout.write("I'm alive!");
+  process.exit(0);
+}, 500);
+
+throw new Error();

packages/node-integration-tests/suites/public-api/OnUncaughtException/mimic-native-behaviour-no-additional-listener-test-script.js

@@ -0,0 +1,24 @@
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: integrations => {
+    return integrations.map(integration => {
+      if (integration.name === 'OnUncaughtException') {
+        return new Sentry.Integrations.OnUncaughtException({
+          exitEvenIfOtherHandlersAreRegistered: false,
+        });
+      } else {
+        return integration;
+      }
+    });
+  },
+});
+
+setTimeout(() => {
+  // This should not be called because the script throws before this resolves
+  process.stdout.write("I'm alive!");
+  process.exit(0);
+}, 500);
+
+throw new Error();

packages/node-integration-tests/suites/public-api/OnUncaughtException/no-additional-listener-test-script.js

@@ -0,0 +1,13 @@
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+});
+
+setTimeout(() => {
+  // This should not be called because the script throws before this resolves
+  process.stdout.write("I'm alive!");
+  process.exit(0);
+}, 500);
+
+throw new Error();

packages/node-integration-tests/suites/public-api/OnUncaughtException/test.ts

@@ -0,0 +1,57 @@
+import * as childProcess from 'child_process';
+import * as path from 'path';
+
+describe('OnUncaughtException integration', () => {
+  test('should close process on uncaught error with no additional listeners registered', done => {
+    expect.assertions(3);
+
+    const testScriptPath = path.resolve(__dirname, 'no-additional-listener-test-script.js');
+
+    childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => {
+      expect(err).not.toBeNull();
+      expect(err?.code).toBe(1);
+      expect(stdout).not.toBe("I'm alive!");
+      done();
+    });
+  });
+
+  test('should close process on uncaught error when additional listeners are registered', done => {
+    expect.assertions(3);
+
+    const testScriptPath = path.resolve(__dirname, 'additional-listener-test-script.js');
+
+    childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => {
+      expect(err).not.toBeNull();
+      expect(err?.code).toBe(1);
+      expect(stdout).not.toBe("I'm alive!");
+      done();
+    });
+  });
+
+  describe('with `exitEvenIfOtherHandlersAreRegistered` set to false', () => {
+    test('should close process on uncaught error with no additional listeners registered', done => {
+      expect.assertions(3);
+
+      const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-no-additional-listener-test-script.js');
+
+      childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => {
+        expect(err).not.toBeNull();
+        expect(err?.code).toBe(1);
+        expect(stdout).not.toBe("I'm alive!");
+        done();
+      });
+    });
+
+    test('should not close process on uncaught error when additional listeners are registered', done => {
+      expect.assertions(2);
+
+      const testScriptPath = path.resolve(__dirname, 'mimic-native-behaviour-additional-listener-test-script.js');
+
+      childProcess.exec(`node ${testScriptPath}`, { encoding: 'utf8' }, (err, stdout) => {
+        expect(err).toBeNull();
+        expect(stdout).toBe("I'm alive!");
+        done();
+      });
+    });
+  });
+});

packages/node/src/integrations/onuncaughtexception.ts

@@ -7,6 +7,30 @@ import { logAndExitProcess } from './utils/errorhandling';
 
 type OnFatalErrorHandler = (firstError: Error, secondError?: Error) => void;
 
+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
+  // falling back to current behaviour when that's not available.
+  /**
+   * Controls if the SDK should register a handler to exit the process on uncaught errors:
+   * - `true`: The SDK will exit the process on all uncaught errors.
+   * - `false`: The SDK will only exit the process when there are no other `uncaughtException` handlers attached.
+   *
+   * Default: `true`
+   */
+  exitEvenIfOtherHandlersAreRegistered: boolean;
+
+  /**
+   * This is called when an uncaught error would cause the process to exit.
+   *
+   * @param firstError Uncaught error causing the process to exit
+   * @param secondError Will be set if the handler was called multiple times. This can happen either because
+   * `onFatalError` itself threw, or because an independent error happened somewhere else while `onFatalError`
+   * was running.
+   */
+  onFatalError?(firstError: Error, secondError?: Error): void;
+}
+
 /** Global Exception handler */
 export class OnUncaughtException implements Integration {
   /**
@@ -24,24 +48,23 @@ export class OnUncaughtException implements Integration {
    */
   public readonly handler: (error: Error) => void = this._makeErrorHandler();
 
+  private readonly _options: OnUncaughtExceptionOptions;
+
   /**
    * @inheritDoc
    */
-  public constructor(
-    private readonly _options: {
-      /**
-       * Default onFatalError handler
-       * @param firstError Error that has been thrown
-       * @param secondError If this was called multiple times this will be set
-       */
-      onFatalError?(firstError: Error, secondError?: Error): void;
-    } = {},
-  ) {}
+  public constructor(options: Partial<OnUncaughtExceptionOptions> = {}) {
+    this._options = {
+      exitEvenIfOtherHandlersAreRegistered: true,
+      ...options,
+    };
+  }
+
   /**
    * @inheritDoc
    */
   public setupOnce(): void {
-    global.process.on('uncaughtException', this.handler.bind(this));
+    global.process.on('uncaughtException', this.handler);
   }
 
   /**
@@ -66,6 +89,27 @@ export class OnUncaughtException implements Integration {
         onFatalError = client.getOptions().onFatalError as OnFatalErrorHandler;
       }
 
+      // Attaching a listener to `uncaughtException` will prevent the node process from exiting. We generally do not
+      // want to alter this behaviour so we check for other listeners that users may have attached themselves and adjust
+      // exit behaviour of the SDK accordingly:
+      // - If other listeners are attached, do not exit.
+      // - If the only listener attached is ours, exit.
+      const userProvidedListenersCount = global.process
+        .listeners('uncaughtException')
+        .reduce<number>((acc, listener) => {
+          if (
+            listener.name === 'domainUncaughtExceptionClear' || // as soon as we're using domains this listener is attached by node itself
+            listener === this.handler // filter the handler we registered ourselves)
+          ) {
+            return acc;
+          } else {
+            return acc + 1;
+          }
+        }, 0);
+
+      const processWouldExit = userProvidedListenersCount === 0;
+      const shouldApplyFatalHandlingLogic = this._options.exitEvenIfOtherHandlersAreRegistered || processWouldExit;
+
       if (!caughtFirstError) {
         const hub = getCurrentHub();
 
@@ -82,47 +126,53 @@ export class OnUncaughtException implements Integration {
               originalException: error,
               data: { mechanism: { handled: false, type: 'onuncaughtexception' } },
             });
-            if (!calledFatalError) {
+            if (!calledFatalError && shouldApplyFatalHandlingLogic) {
               calledFatalError = true;
               onFatalError(error);
             }
           });
         } else {
-          if (!calledFatalError) {
+          if (!calledFatalError && shouldApplyFatalHandlingLogic) {
             calledFatalError = true;
             onFatalError(error);
           }
         }
-      } else if (calledFatalError) {
-        // we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down
-        __DEBUG_BUILD__ &&
-          logger.warn('uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown');
-        logAndExitProcess(error);
-      } else if (!caughtSecondError) {
-        // two cases for how we can hit this branch:
-        //   - capturing of first error blew up and we just caught the exception from that
-        //     - quit trying to capture, proceed with shutdown
-        //   - a second independent error happened while waiting for first error to capture
-        //     - want to avoid causing premature shutdown before first error capture finishes
-        // it's hard to immediately tell case 1 from case 2 without doing some fancy/questionable domain stuff
-        // so let's instead just delay a bit before we proceed with our action here
-        // in case 1, we just wait a bit unnecessarily but ultimately do the same thing
-        // in case 2, the delay hopefully made us wait long enough for the capture to finish
-        // two potential nonideal outcomes:
-        //   nonideal case 1: capturing fails fast, we sit around for a few seconds unnecessarily before proceeding correctly by calling onFatalError
-        //   nonideal case 2: case 2 happens, 1st error is captured but slowly, timeout completes before capture and we treat second error as the sendErr of (nonexistent) failure from trying to capture first error
-        // note that after hitting this branch, we might catch more errors where (caughtSecondError && !calledFatalError)
-        //   we ignore them - they don't matter to us, we're just waiting for the second error timeout to finish
-        caughtSecondError = true;
-        setTimeout(() => {
-          if (!calledFatalError) {
-            // it was probably case 1, let's treat err as the sendErr and call onFatalError
-            calledFatalError = true;
-            onFatalError(firstError, error);
-          } else {
-            // it was probably case 2, our first error finished capturing while we waited, cool, do nothing
+      } else {
+        if (shouldApplyFatalHandlingLogic) {
+          if (calledFatalError) {
+            // we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down
+            __DEBUG_BUILD__ &&
+              logger.warn(
+                'uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown',
+              );
+            logAndExitProcess(error);
+          } else if (!caughtSecondError) {
+            // two cases for how we can hit this branch:
+            //   - capturing of first error blew up and we just caught the exception from that
+            //     - quit trying to capture, proceed with shutdown
+            //   - a second independent error happened while waiting for first error to capture
+            //     - want to avoid causing premature shutdown before first error capture finishes
+            // it's hard to immediately tell case 1 from case 2 without doing some fancy/questionable domain stuff
+            // so let's instead just delay a bit before we proceed with our action here
+            // in case 1, we just wait a bit unnecessarily but ultimately do the same thing
+            // in case 2, the delay hopefully made us wait long enough for the capture to finish
+            // two potential nonideal outcomes:
+            //   nonideal case 1: capturing fails fast, we sit around for a few seconds unnecessarily before proceeding correctly by calling onFatalError
+            //   nonideal case 2: case 2 happens, 1st error is captured but slowly, timeout completes before capture and we treat second error as the sendErr of (nonexistent) failure from trying to capture first error
+            // note that after hitting this branch, we might catch more errors where (caughtSecondError && !calledFatalError)
+            //   we ignore them - they don't matter to us, we're just waiting for the second error timeout to finish
+            caughtSecondError = true;
+            setTimeout(() => {
+              if (!calledFatalError) {
+                // it was probably case 1, let's treat err as the sendErr and call onFatalError
+                calledFatalError = true;
+                onFatalError(firstError, error);
+              } else {
+                // it was probably case 2, our first error finished capturing while we waited, cool, do nothing
+              }
+            }, timeout); // capturing could take at least sendTimeout to fail, plus an arbitrary second for how long it takes to collect surrounding source etc
           }
-        }, timeout); // capturing could take at least sendTimeout to fail, plus an arbitrary second for how long it takes to collect surrounding source etc
+        }
       }
     };
   }