ref(node): Refactor node integrations to avoid setupOnce

mydea · mydea · commit 9b516cbd4330 · 2023-12-19T14:25:26.000+01:00
diff --git a/packages/node/src/integrations/console.ts b/packages/node/src/integrations/console.ts
@@ -1,6 +1,6 @@
 import * as util from 'util';
-import { addBreadcrumb, getCurrentHub } from '@sentry/core';
-import type { Integration } from '@sentry/types';
+import { addBreadcrumb, getClient } from '@sentry/core';
+import type { Client, Integration } from '@sentry/types';
 import { addConsoleInstrumentationHandler, severityLevelFromString } from '@sentry/utils';
 
 /** Console module integration */
@@ -19,10 +19,13 @@ export class Console implements Integration {
    * @inheritDoc
    */
   public setupOnce(): void {
-    addConsoleInstrumentationHandler(({ args, level }) => {
-      const hub = getCurrentHub();
+    // noop
+  }
 
-      if (!hub.getIntegration(Console)) {
+  /** @inheritdoc */
+  public setup(client: Client): void {
+    addConsoleInstrumentationHandler(({ args, level }) => {
+      if (getClient() !== client) {
         return;
       }
 
diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts
@@ -1,5 +1,5 @@
-import type { Scope } from '@sentry/core';
-import { getClient, getCurrentHub } from '@sentry/core';
+import { captureException } from '@sentry/core';
+import { getClient } from '@sentry/core';
 import type { Integration } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
@@ -50,11 +50,6 @@ export class OnUncaughtException implements Integration {
    */
   public name: string = OnUncaughtException.id;
 
-  /**
-   * @inheritDoc
-   */
-  public readonly handler: (error: Error) => void = this._makeErrorHandler();
-
   // CAREFUL: Please think twice before updating the way _options looks because the Next.js SDK depends on it in `index.server.ts`
   private readonly _options: OnUncaughtExceptionOptions;
 
@@ -68,31 +63,46 @@ export class OnUncaughtException implements Integration {
     };
   }
 
+  /**
+   * @deprecated This does nothing anymore.
+   */
+  public readonly handler: (error: Error) => void = () => {
+    // noop
+  };
+
   /**
    * @inheritDoc
    */
   public setupOnce(): void {
-    global.process.on('uncaughtException', this.handler);
+    // noop
   }
 
-  /**
-   * @hidden
-   */
-  private _makeErrorHandler(): (error: Error) => void {
-    const timeout = 2000;
-    let caughtFirstError: boolean = false;
-    let caughtSecondError: boolean = false;
-    let calledFatalError: boolean = false;
-    let firstError: Error;
-
-    return (error: Error): void => {
+  /** @inheritdoc */
+  public setup(client: NodeClient): void {
+    global.process.on('uncaughtException', makeErrorHandler(client, this._options));
+  }
+}
+
+type ErrorHandler = { _errorHandler: boolean } & ((error: Error) => void);
+
+/** Exported only for tests */
+export function makeErrorHandler(client: NodeClient, options: OnUncaughtExceptionOptions): ErrorHandler {
+  const timeout = 2000;
+  let caughtFirstError: boolean = false;
+  let caughtSecondError: boolean = false;
+  let calledFatalError: boolean = false;
+  let firstError: Error;
+
+  const clientOptions = client.getOptions();
+
+  return Object.assign(
+    (error: Error): void => {
       let onFatalError: OnFatalErrorHandler = logAndExitProcess;
-      const client = getClient<NodeClient>();
 
-      if (this._options.onFatalError) {
-        onFatalError = this._options.onFatalError;
-      } else if (client && client.getOptions().onFatalError) {
-        onFatalError = client.getOptions().onFatalError as OnFatalErrorHandler;
+      if (options.onFatalError) {
+        onFatalError = options.onFatalError;
+      } else if (clientOptions.onFatalError) {
+        onFatalError = clientOptions.onFatalError as OnFatalErrorHandler;
       }
 
       // Attaching a listener to `uncaughtException` will prevent the node process from exiting. We generally do not
@@ -107,7 +117,7 @@ export class OnUncaughtException implements Integration {
           // There are 3 listeners we ignore:
           listener.name === 'domainUncaughtExceptionClear' || // as soon as we're using domains this listener is attached by node itself
           (listener.tag && listener.tag === 'sentry_tracingErrorCallback') || // the handler we register for tracing
-          listener === this.handler // the handler we register in this integration
+          (listener as ErrorHandler)._errorHandler // the handler we register in this integration
         ) {
           return acc;
         } else {
@@ -116,34 +126,31 @@ export class OnUncaughtException implements Integration {
       }, 0);
 
       const processWouldExit = userProvidedListenersCount === 0;
-      const shouldApplyFatalHandlingLogic = this._options.exitEvenIfOtherHandlersAreRegistered || processWouldExit;
+      const shouldApplyFatalHandlingLogic = options.exitEvenIfOtherHandlersAreRegistered || processWouldExit;
 
       if (!caughtFirstError) {
-        const hub = getCurrentHub();
-
         // this is the first uncaught error and the ultimate reason for shutting down
         // we want to do absolutely everything possible to ensure it gets captured
         // also we want to make sure we don't go recursion crazy if more errors happen after this one
         firstError = error;
         caughtFirstError = true;
 
-        if (hub.getIntegration(OnUncaughtException)) {
-          hub.withScope((scope: Scope) => {
-            scope.setLevel('fatal');
-            hub.captureException(error, {
-              originalException: error,
-              data: { mechanism: { handled: false, type: 'onuncaughtexception' } },
-            });
-            if (!calledFatalError && shouldApplyFatalHandlingLogic) {
-              calledFatalError = true;
-              onFatalError(error);
-            }
+        if (getClient() === client) {
+          captureException(error, {
+            originalException: error,
+            captureContext: {
+              level: 'fatal',
+            },
+            mechanism: {
+              handled: false,
+              type: 'onuncaughtexception',
+            },
           });
-        } else {
-          if (!calledFatalError && shouldApplyFatalHandlingLogic) {
-            calledFatalError = true;
-            onFatalError(error);
-          }
+        }
+
+        if (!calledFatalError && shouldApplyFatalHandlingLogic) {
+          calledFatalError = true;
+          onFatalError(error);
         }
       } else {
         if (shouldApplyFatalHandlingLogic) {
@@ -182,6 +189,7 @@ export class OnUncaughtException implements Integration {
           }
         }
       }
-    };
-  }
+    },
+    { _errorHandler: true },
+  );
 }
diff --git a/packages/node/src/integrations/onunhandledrejection.ts b/packages/node/src/integrations/onunhandledrejection.ts
@@ -1,12 +1,19 @@
-import type { Scope } from '@sentry/core';
-import { getCurrentHub } from '@sentry/core';
-import type { Integration } from '@sentry/types';
+import { captureException, getClient } from '@sentry/core';
+import type { Client, Integration } from '@sentry/types';
 import { consoleSandbox } from '@sentry/utils';
 
 import { logAndExitProcess } from './utils/errorhandling';
 
 type UnhandledRejectionMode = 'none' | 'warn' | 'strict';
 
+interface OnUnhandledRejectionOptions {
+  /**
+   * Option deciding what to do after capturing unhandledRejection,
+   * that mimicks behavior of node's --unhandled-rejection flag.
+   */
+  mode: UnhandledRejectionMode;
+}
+
 /** Global Promise Rejection handler */
 export class OnUnhandledRejection implements Integration {
   /**
@@ -22,67 +29,80 @@ export class OnUnhandledRejection implements Integration {
   /**
    * @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(private readonly _options: OnUnhandledRejectionOptions = { mode: 'warn' }) {}
 
   /**
    * @inheritDoc
    */
   public setupOnce(): void {
-    global.process.on('unhandledRejection', this.sendUnhandledPromise.bind(this));
+    // noop
   }
 
-  /**
-   * Send an exception with reason
-   * @param reason string
-   * @param promise promise
-   */
-  public sendUnhandledPromise(reason: unknown, promise: unknown): void {
-    const hub = getCurrentHub();
-    if (hub.getIntegration(OnUnhandledRejection)) {
-      hub.withScope((scope: Scope) => {
-        scope.setExtra('unhandledPromiseRejection', true);
-        hub.captureException(reason, {
-          originalException: promise,
-          data: { mechanism: { handled: false, type: 'onunhandledrejection' } },
-        });
-      });
-    }
-    this._handleRejection(reason);
+  /** @inheritdoc */
+  public setup(client: Client): void {
+    global.process.on('unhandledRejection', makeUnhandledPromiseHandler(client, this._options));
   }
+}
 
-  /**
-   * Handler for `mode` option
-   */
+/**
+ * Send an exception with reason
+ * @param reason string
+ * @param promise promise
+ *
+ * Exported only for tests.
+ */
+export function makeUnhandledPromiseHandler(
+  client: Client,
+  options: OnUnhandledRejectionOptions,
+): (reason: unknown, promise: unknown) => void {
+  return function sendUnhandledPromise(reason: unknown, promise: unknown): void {
+    if (getClient() !== client) {
+      return;
+    }
+
+    captureException(reason, {
+      originalException: promise,
+      captureContext: {
+        extra: { unhandledPromiseRejection: true },
+      },
+      mechanism: {
+        handled: false,
+        type: 'onunhandledrejection',
+      },
+    });
+
+    handleRejection(reason, options);
+  };
+}
+
+/**
+ * Handler for `mode` option
+
+ */
+function handleRejection(
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  private _handleRejection(reason: any): void {
-    // https://github.com/nodejs/node/blob/7cf6f9e964aa00772965391c23acda6d71972a9a/lib/internal/process/promises.js#L234-L240
-    const rejectionWarning =
-      'This error originated either by ' +
-      'throwing inside of an async function without a catch block, ' +
-      'or by rejecting a promise which was not handled with .catch().' +
-      ' The promise rejected with the reason:';
+  reason: any,
+  options: OnUnhandledRejectionOptions,
+): void {
+  // https://github.com/nodejs/node/blob/7cf6f9e964aa00772965391c23acda6d71972a9a/lib/internal/process/promises.js#L234-L240
+  const rejectionWarning =
+    'This error originated either by ' +
+    'throwing inside of an async function without a catch block, ' +
+    'or by rejecting a promise which was not handled with .catch().' +
+    ' The promise rejected with the reason:';
 
-    /* eslint-disable no-console */
-    if (this._options.mode === 'warn') {
-      consoleSandbox(() => {
-        console.warn(rejectionWarning);
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-        console.error(reason && reason.stack ? reason.stack : reason);
-      });
-    } else if (this._options.mode === 'strict') {
-      consoleSandbox(() => {
-        console.warn(rejectionWarning);
-      });
-      logAndExitProcess(reason);
-    }
-    /* eslint-enable no-console */
+  /* eslint-disable no-console */
+  if (options.mode === 'warn') {
+    consoleSandbox(() => {
+      console.warn(rejectionWarning);
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+      console.error(reason && reason.stack ? reason.stack : reason);
+    });
+  } else if (options.mode === 'strict') {
+    consoleSandbox(() => {
+      console.warn(rejectionWarning);
+    });
+    logAndExitProcess(reason);
   }
+  /* eslint-enable no-console */
 }
diff --git a/packages/node/test/onuncaughtexception.test.ts b/packages/node/test/onuncaughtexception.test.ts
@@ -1,34 +1,50 @@
-import { Hub } from '@sentry/core';
+import * as SentryCore from '@sentry/core';
+import type { NodeClient } from '../src/client';
 
-import { OnUncaughtException } from '../src/integrations/onuncaughtexception';
+import { OnUncaughtException, makeErrorHandler } from '../src/integrations/onuncaughtexception';
+
+const client = {
+  getOptions: () => ({}),
+  close: () => Promise.resolve(true),
+} as unknown as NodeClient;
 
 jest.mock('@sentry/core', () => {
   // we just want to short-circuit it, so dont worry about types
   const original = jest.requireActual('@sentry/core');
-  original.Hub.prototype.getIntegration = () => true;
   return {
     ...original,
-    getCurrentHub: () => new Hub(),
+    getClient: () => client,
   };
 });
 
 describe('uncaught exceptions', () => {
   test('install global listener', () => {
     const integration = new OnUncaughtException();
-    integration.setupOnce();
+    integration.setup(client);
     expect(process.listeners('uncaughtException')).toHaveLength(1);
   });
 
-  test('sendUncaughtException', () => {
-    const integration = new OnUncaughtException({ onFatalError: jest.fn() });
-    integration.setupOnce();
-
-    const captureException = jest.spyOn(Hub.prototype, 'captureException');
+  test('makeErrorHandler', () => {
+    const captureExceptionMock = jest.spyOn(SentryCore, 'captureException');
+    const handler = makeErrorHandler(client, {
+      exitEvenIfOtherHandlersAreRegistered: true,
+      onFatalError: () => {},
+    });
 
-    integration.handler({ message: 'message', name: 'name' });
+    handler({ message: 'message', name: 'name' });
 
-    expect(captureException.mock.calls[0][1]?.data).toEqual({
-      mechanism: { handled: false, type: 'onuncaughtexception' },
+    expect(captureExceptionMock.mock.calls[0][1]).toEqual({
+      originalException: {
+        message: 'message',
+        name: 'name',
+      },
+      captureContext: {
+        level: 'fatal',
+      },
+      mechanism: {
+        handled: false,
+        type: 'onuncaughtexception',
+      },
     });
   });
 });
diff --git a/packages/node/test/onunhandledrejection.test.ts b/packages/node/test/onunhandledrejection.test.ts
