fix: Only import inspector asynchronously (#12231)

Closes #12223 I've added a test that uses `import-in-the-middle` to throw if `inspector` is imported without enabling the `LocalVariables` integration so this should not regress in the future. Note that we don't need to import asynchronously in the worker scripts since these are not evaluated if the features aren't used.
getsentry · May 27, 2024 · 5282753 · 5282753
1 parent 1f8f1b8
commit 5282753
Show file tree

Hide file tree

Showing 6 changed files with 135 additions and 110 deletions.
diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/deny-inspector.mjs b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/deny-inspector.mjs
@@ -0,0 +1,13 @@
+import * as Sentry from '@sentry/node';
+import Hook from 'import-in-the-middle';
+
+Hook((_, name) => {
+  if (name === 'inspector') {
+    throw new Error('No inspector!');
+  }
+  if (name === 'node:inspector') {
+    throw new Error('No inspector!');
+  }
+});
+
+Sentry.init({});
diff --git a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts
@@ -76,6 +76,14 @@ conditionalTest({ min: 18 })('LocalVariables integration', () => {
       .start(done);
   });
 
+  test('Should not import inspector when not in use', done => {
+    createRunner(__dirname, 'deny-inspector.mjs')
+      .withFlags('--import=@sentry/node/import')
+      .ensureNoErrorOutput()
+      .ignore('session')
+      .start(done);
+  });
+
   test('Includes local variables for caught exceptions when enabled', done => {
     createRunner(__dirname, 'local-variables-caught.js')
       .ignore('session')

diff --git a/packages/node/src/integrations/anr/index.ts b/packages/node/src/integrations/anr/index.ts
@@ -1,4 +1,3 @@
-import * as inspector from 'node:inspector';
 import { Worker } from 'node:worker_threads';
 import { defineIntegration, getCurrentScope, getGlobalScope, getIsolationScope, mergeScopeData } from '@sentry/core';
 import type { Contexts, Event, EventHint, Integration, IntegrationFn, ScopeData } from '@sentry/types';
@@ -148,6 +147,7 @@ async function _startWorker(
   };
 
   if (options.captureStackTrace) {
+    const inspector = await import('node:inspector');
     if (!inspector.url()) {
       inspector.open(0);
     }

diff --git a/packages/node/src/integrations/anr/worker.ts b/packages/node/src/integrations/anr/worker.ts
@@ -1,3 +1,4 @@
+import { Session as InspectorSession } from 'node:inspector';
 import { parentPort, workerData } from 'node:worker_threads';
 import {
   applyScopeDataToEvent,
@@ -15,7 +16,6 @@ import {
   uuid4,
   watchdogTimer,
 } from '@sentry/utils';
-import { Session as InspectorSession } from 'inspector';
 
 import { makeNodeTransport } from '../../transports';
 import { createGetModuleFromFilename } from '../../utils/module';

diff --git a/packages/node/src/integrations/local-variables/local-variables-async.ts b/packages/node/src/integrations/local-variables/local-variables-async.ts
@@ -75,7 +75,7 @@ export const localVariablesAsyncIntegration = defineIntegration(((
 
   async function startInspector(): Promise<void> {
     // We load inspector dynamically because on some platforms Node is built without inspector support
-    const inspector = await import('inspector');
+    const inspector = await import('node:inspector');
     if (!inspector.url()) {
       inspector.open(0);
     }

diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts
@@ -1,5 +1,4 @@
-import type { Debugger, InspectorNotification, Runtime } from 'node:inspector';
-import { Session } from 'node:inspector';
+import type { Debugger, InspectorNotification, Runtime, Session } from 'node:inspector';
 import { defineIntegration, getClient } from '@sentry/core';
 import type { Event, Exception, IntegrationFn, StackParser } from '@sentry/types';
 import { LRUMap, logger } from '@sentry/utils';
@@ -75,11 +74,18 @@ export function createCallbackList<T>(complete: Next<T>): CallbackWrapper<T> {
  * https://nodejs.org/docs/latest-v14.x/api/inspector.html
  */
 class AsyncSession implements DebugSession {
-  private readonly _session: Session;
-
   /** Throws if inspector API is not available */
-  public constructor() {
-    this._session = new Session();
+  private constructor(private readonly _session: Session) {
+    //
+  }
+
+  public static async create(orDefault?: DebugSession | undefined): Promise<DebugSession> {
+    if (orDefault) {
+      return orDefault;
+    }
+
+    const inspector = await import('node:inspector');
+    return new AsyncSession(new inspector.Session());
   }
 
   /** @inheritdoc */
@@ -194,85 +200,19 @@ class AsyncSession implements DebugSession {
   }
 }
 
-/**
- * When using Vercel pkg, the inspector module is not available.
- * https://github.com/getsentry/sentry-javascript/issues/6769
- */
-function tryNewAsyncSession(): AsyncSession | undefined {
-  try {
-    return new AsyncSession();
-  } catch (e) {
-    return undefined;
-  }
-}
-
 const INTEGRATION_NAME = 'LocalVariables';
 
 /**
  * Adds local variables to exception frames
  */
 const _localVariablesSyncIntegration = ((
   options: LocalVariablesIntegrationOptions = {},
-  session: DebugSession | undefined = tryNewAsyncSession(),
+  sessionOverride?: DebugSession,
 ) => {
   const cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
   let rateLimiter: RateLimitIncrement | undefined;
   let shouldProcessEvent = false;
 
-  function handlePaused(
-    stackParser: StackParser,
-    { params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
-    complete: () => void,
-  ): void {
-    if (reason !== 'exception' && reason !== 'promiseRejection') {
-      complete();
-      return;
-    }
-
-    rateLimiter?.();
-
-    // data.description contains the original error.stack
-    const exceptionHash = hashFromStack(stackParser, data?.description);
-
-    if (exceptionHash == undefined) {
-      complete();
-      return;
-    }
-
-    const { add, next } = createCallbackList<FrameVariables[]>(frames => {
-      cachedFrames.set(exceptionHash, frames);
-      complete();
-    });
-
-    // Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
-    // For this reason we only attempt to get local variables for the first 5 frames
-    for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
-      const { scopeChain, functionName, this: obj } = callFrames[i];
-
-      const localScope = scopeChain.find(scope => scope.type === 'local');
-
-      // obj.className is undefined in ESM modules
-      const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;
-
-      if (localScope?.object.objectId === undefined) {
-        add(frames => {
-          frames[i] = { function: fn };
-          next(frames);
-        });
-      } else {
-        const id = localScope.object.objectId;
-        add(frames =>
-          session?.getLocalVariables(id, vars => {
-            frames[i] = { function: fn, vars };
-            next(frames);
-          }),
-        );
-      }
-    }
-
-    next([]);
-  }
-
   function addLocalVariablesToException(exception: Exception): void {
     const hash = hashFrames(exception?.stacktrace?.frames);
 
@@ -330,44 +270,108 @@ const _localVariablesSyncIntegration = ((
       const client = getClient<NodeClient>();
       const clientOptions = client?.getOptions();
 
-      if (session && clientOptions?.includeLocalVariables) {
-        // Only setup this integration if the Node version is >= v18
-        // https://github.com/getsentry/sentry-javascript/issues/7697
-        const unsupportedNodeVersion = NODE_MAJOR < 18;
+      if (!clientOptions?.includeLocalVariables) {
+        return;
+      }
 
-        if (unsupportedNodeVersion) {
-          logger.log('The `LocalVariables` integration is only supported on Node >= v18.');
-          return;
-        }
+      // Only setup this integration if the Node version is >= v18
+      // https://github.com/getsentry/sentry-javascript/issues/7697
+      const unsupportedNodeVersion = NODE_MAJOR < 18;
 
-        const captureAll = options.captureAllExceptions !== false;
-
-        session.configureAndConnect(
-          (ev, complete) =>
-            handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
-          captureAll,
-        );
-
-        if (captureAll) {
-          const max = options.maxExceptionsPerSecond || 50;
-
-          rateLimiter = createRateLimiter(
-            max,
-            () => {
-              logger.log('Local variables rate-limit lifted.');
-              session?.setPauseOnExceptions(true);
-            },
-            seconds => {
-              logger.log(
-                `Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
-              );
-              session?.setPauseOnExceptions(false);
-            },
+      if (unsupportedNodeVersion) {
+        logger.log('The `LocalVariables` integration is only supported on Node >= v18.');
+        return;
+      }
+
+      AsyncSession.create(sessionOverride).then(
+        session => {
+          function handlePaused(
+            stackParser: StackParser,
+            { params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
+            complete: () => void,
+          ): void {
+            if (reason !== 'exception' && reason !== 'promiseRejection') {
+              complete();
+              return;
+            }
+
+            rateLimiter?.();
+
+            // data.description contains the original error.stack
+            const exceptionHash = hashFromStack(stackParser, data?.description);
+
+            if (exceptionHash == undefined) {
+              complete();
+              return;
+            }
+
+            const { add, next } = createCallbackList<FrameVariables[]>(frames => {
+              cachedFrames.set(exceptionHash, frames);
+              complete();
+            });
+
+            // Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
+            // For this reason we only attempt to get local variables for the first 5 frames
+            for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
+              const { scopeChain, functionName, this: obj } = callFrames[i];
+
+              const localScope = scopeChain.find(scope => scope.type === 'local');
+
+              // obj.className is undefined in ESM modules
+              const fn =
+                obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;
+
+              if (localScope?.object.objectId === undefined) {
+                add(frames => {
+                  frames[i] = { function: fn };
+                  next(frames);
+                });
+              } else {
+                const id = localScope.object.objectId;
+                add(frames =>
+                  session?.getLocalVariables(id, vars => {
+                    frames[i] = { function: fn, vars };
+                    next(frames);
+                  }),
+                );
+              }
+            }
+
+            next([]);
+          }
+
+          const captureAll = options.captureAllExceptions !== false;
+
+          session.configureAndConnect(
+            (ev, complete) =>
+              handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
+            captureAll,
           );
-        }
 
-        shouldProcessEvent = true;
-      }
+          if (captureAll) {
+            const max = options.maxExceptionsPerSecond || 50;
+
+            rateLimiter = createRateLimiter(
+              max,
+              () => {
+                logger.log('Local variables rate-limit lifted.');
+                session?.setPauseOnExceptions(true);
+              },
+              seconds => {
+                logger.log(
+                  `Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
+                );
+                session?.setPauseOnExceptions(false);
+              },
+            );
+          }
+
+          shouldProcessEvent = true;
+        },
+        error => {
+          logger.log('The `LocalVariables` integration failed to start.', error);
+        },
+      );
     },
     processEvent(event: Event): Event {
       if (shouldProcessEvent) {