Sentry Node: do not exit process if the app added its own 'uncaughtException' or 'unhandledRejection' listener

packages/node-integration-tests/suites/public-api/integrations/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/integrations/OnUncaughtException/mimic-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({
+          mimicNativeBehaviour: true,
+        });
+      } 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/integrations/OnUncaughtException/mimic-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({
+          mimicNativeBehaviour: true,
+        });
+      } 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/integrations/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/integrations/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 `mimicNativeBehaviour` set to true', () => {
+    test('should close process on uncaught error with no additional listeners registered', done => {
+      expect.assertions(3);
+
+      const testScriptPath = path.resolve(__dirname, 'mimic-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-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-integration-tests/suites/public-api/integrations/OnUnhandledRejection/test-script.js

@@ -0,0 +1,29 @@
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: integrations => {
+    return integrations.map(integration => {
+      if (integration.name === 'OnUnhandledRejection') {
+        return new Sentry.Integrations.OnUnhandledRejection({
+          mode: process.env['PROMISE_REJECTION_MODE'],
+        });
+      } else {
+        return integration;
+      }
+    });
+  },
+});
+
+if (process.env['ATTACH_ADDITIONAL_HANDLER']) {
+  process.on('uncaughtException', () => {
+    // do nothing - this will prevent the rejected promise below from closing this process before the timeout resolves
+  });
+}
+
+setTimeout(() => {
+  process.stdout.write("I'm alive!");
+  process.exit(0);
+}, 500);
+
+Promise.reject();

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

@@ -0,0 +1,36 @@
+import * as childProcess from 'child_process';
+import * as path from 'path';
+
+describe('OnUnhandledRejection integration', () => {
+  test.each([
+    { mode: 'none', additionalHandler: false, expectedErrorCode: 0 },
+    { mode: 'warn', additionalHandler: false, expectedErrorCode: 0 },
+    { mode: 'strict', additionalHandler: false, expectedErrorCode: 1 },
+
+    { mode: 'none', additionalHandler: true, expectedErrorCode: 0 },
+    { mode: 'warn', additionalHandler: true, expectedErrorCode: 0 },
+    { mode: 'strict', additionalHandler: true, expectedErrorCode: 1 },
+  ])(
+    'should cause process to exit with code $expectedErrorCode with `mode` set to $mode while having a handler attached (? - $additionalHandler)',
+    async ({ mode, additionalHandler, expectedErrorCode }) => {
+      const testScriptPath = path.resolve(__dirname, 'test-script.js');
+
+      await new Promise(resolve => {
+        childProcess.exec(
+          `node ${testScriptPath}`,
+          {
+            encoding: 'utf8',
+            env: {
+              PROMISE_REJECTION_MODE: mode ? String(mode) : undefined,
+              ATTACH_ADDITIONAL_HANDLER: additionalHandler ? String(additionalHandler) : undefined,
+            },
+          },
+          err => {
+            expect(err?.code).toBe(expectedErrorCode || undefined);
+            resolve();
+          },
+        );
+      });
+    },
+  );
+});

packages/node/src/integrations/onuncaughtexception.ts

@@ -8,6 +8,28 @@ 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.
+  /**
+   * Whether the SDK should mimic native behaviour when a global error occurs. Default: `false`
+   * - `false`: The SDK will exit the process on all uncaught errors.
+   * - `true`: The SDK will only exit the process when there are no other 'uncaughtException' handlers attached.
+   */
+  mimicNativeBehaviour: 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 Promise Rejection handler */
 export class OnUncaughtException implements Integration {
   /**
@@ -23,26 +45,25 @@ export class OnUncaughtException implements Integration {
   /**
    * @inheritDoc
    */
-  public readonly handler: (error: Error) => void = this._makeErrorHandler();
+  public readonly handler: (error: Error) => void = this._makeErrorHandler().bind(this);
+
+  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 = {
+      mimicNativeBehaviour: false,
+      ...options,
+    };
+  }
+
   /**
    * @inheritDoc
    */
   public setupOnce(): void {
-    global.process.on('uncaughtException', this.handler.bind(this));
+    global.process.on('uncaughtException', this.handler);
   }
 
   /**
@@ -59,6 +80,26 @@ export class OnUncaughtException implements Integration {
       let onFatalError: OnFatalErrorHandler = logAndExitProcess;
       const client = getCurrentHub().getClient<NodeClient>();
 
+      // 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 shouldExitProcess: boolean = !this._options.mimicNativeBehaviour || userProvidedListenersCount === 0;
+
       if (this._options.onFatalError) {
         // eslint-disable-next-line @typescript-eslint/unbound-method
         onFatalError = this._options.onFatalError;
@@ -83,23 +124,23 @@ export class OnUncaughtException implements Integration {
               originalException: error,
               data: { mechanism: { handled: false, type: 'onuncaughtexception' } },
             });
-            if (!calledFatalError) {
+            if (!calledFatalError && shouldExitProcess) {
               calledFatalError = true;
               onFatalError(error);
             }
           });
         } else {
-          if (!calledFatalError) {
+          if (!calledFatalError && shouldExitProcess) {
             calledFatalError = true;
             onFatalError(error);
           }
         }
-      } else if (calledFatalError) {
+      } else if (calledFatalError && shouldExitProcess) {
         // we hit an error *after* calling onFatalError - pretty boned at this point, just shut it down
         IS_DEBUG_BUILD &&
           logger.warn('uncaught exception after calling fatal error shutdown callback - this is bad! forcing shutdown');
         logAndExitProcess(error);
-      } else if (!caughtSecondError) {
+      } else if (!caughtSecondError && shouldExitProcess) {
         // 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

packages/node/src/integrations/onunhandledrejection.ts

@@ -4,7 +4,16 @@ import { consoleSandbox } from '@sentry/utils';
 
 import { logAndExitProcess } from './utils/errorhandling';
 
-type UnhandledRejectionMode = 'none' | 'warn' | 'strict';
+interface OnUnhandledRejectionOptions {
+  // TODO(v8): Evaluate whether we should mimic nodes default behaviour for this integration and/or remove the mode option.
+  // 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.
+  /**
+   * Option deciding what to do after capturing unhandledRejection,
+   * that mimicks behavior of node's --unhandled-rejection flag. Default: `'warn'`
+   */
+  mode: 'none' | 'warn' | 'strict';
+}
 
 /** Global Promise Rejection handler */
 export class OnUnhandledRejection implements Integration {
@@ -18,18 +27,17 @@ export class OnUnhandledRejection implements Integration {
    */
   public name: string = OnUnhandledRejection.id;
 
+  private readonly _options: OnUnhandledRejectionOptions;
+
   /**
    * @inheritDoc
    */
-  public constructor(
-    private readonly _options: {
-      /**
-       * Option deciding what to do after capturing unhandledRejection,
-       * that mimicks behavior of node's --unhandled-rejection flag.
-       */
-      mode: UnhandledRejectionMode;
-    } = { mode: 'warn' },
-  ) {}
+  public constructor(options: Partial<OnUnhandledRejectionOptions> = {}) {
+    this._options = {
+      mode: 'warn',
+      ...options,
+    };
+  }
 
   /**
    * @inheritDoc