From d6d0462b32a1d68505aca17af14503207c05c3f2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 28 Aug 2024 13:00:34 +0100 Subject: [PATCH 1/8] fix(node): Remove race conditions and ambiguity when matching local variables --- .../integrations/local-variables/common.ts | 29 ++----- .../local-variables/inspector.d.ts | 4 + .../local-variables/local-variables-async.ts | 60 ++++++-------- .../local-variables/local-variables-sync.ts | 82 +++++++++---------- .../integrations/local-variables/worker.ts | 27 +++--- 5 files changed, 83 insertions(+), 119 deletions(-) diff --git a/packages/node/src/integrations/local-variables/common.ts b/packages/node/src/integrations/local-variables/common.ts index 67c8d6d43d81..58ccea70d6de 100644 --- a/packages/node/src/integrations/local-variables/common.ts +++ b/packages/node/src/integrations/local-variables/common.ts @@ -1,10 +1,14 @@ import type { Debugger } from 'node:inspector'; -import type { StackFrame, StackParser } from '@sentry/types'; export type Variables = Record; export type RateLimitIncrement = () => void; +/** + * The key used to store the local variables on the error object. + */ +export const LOCAL_VARIABLES_KEY = '__SENTRY_ERROR_LOCAL_VARIABLES__'; + /** * Creates a rate limiter that will call the disable callback when the rate limit is reached and the enable callback * when a timeout has occurred. @@ -55,6 +59,7 @@ export type PausedExceptionEvent = Debugger.PausedEventDataType & { data: { // This contains error.stack description: string; + objectId?: string; }; }; @@ -68,28 +73,6 @@ export function functionNamesMatch(a: string | undefined, b: string | undefined) return a === b || (isAnonymous(a) && isAnonymous(b)); } -/** Creates a unique hash from stack frames */ -export function hashFrames(frames: StackFrame[] | undefined): string | undefined { - if (frames === undefined) { - return; - } - - // Only hash the 10 most recent frames (ie. the last 10) - return frames.slice(-10).reduce((acc, frame) => `${acc},${frame.function},${frame.lineno},${frame.colno}`, ''); -} - -/** - * We use the stack parser to create a unique hash from the exception stack trace - * This is used to lookup vars when the exception passes through the event processor - */ -export function hashFromStack(stackParser: StackParser, stack: string | undefined): string | undefined { - if (stack === undefined) { - return undefined; - } - - return hashFrames(stackParser(stack, 1)); -} - export interface FrameVariables { function: string; vars?: Variables; diff --git a/packages/node/src/integrations/local-variables/inspector.d.ts b/packages/node/src/integrations/local-variables/inspector.d.ts index 9ac6b857dcc0..03d9652b2670 100644 --- a/packages/node/src/integrations/local-variables/inspector.d.ts +++ b/packages/node/src/integrations/local-variables/inspector.d.ts @@ -20,6 +20,10 @@ declare module 'node:inspector/promises' { method: 'Runtime.getProperties', params: Runtime.GetPropertiesParameterType, ): Promise; + public post( + method: 'Runtime.callFunctionOn', + params: Runtime.CallFunctionOnParameterType, + ): Promise; public on( event: 'Debugger.paused', diff --git a/packages/node/src/integrations/local-variables/local-variables-async.ts b/packages/node/src/integrations/local-variables/local-variables-async.ts index 86ce9359a95e..cb788ad572db 100644 --- a/packages/node/src/integrations/local-variables/local-variables-async.ts +++ b/packages/node/src/integrations/local-variables/local-variables-async.ts @@ -1,11 +1,12 @@ import { Worker } from 'node:worker_threads'; import { defineIntegration } from '@sentry/core'; -import type { Event, Exception, IntegrationFn } from '@sentry/types'; -import { LRUMap, logger } from '@sentry/utils'; +import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types'; +import { logger } from '@sentry/utils'; import type { NodeClient } from '../../sdk/client'; import type { FrameVariables, LocalVariablesIntegrationOptions, LocalVariablesWorkerArgs } from './common'; -import { functionNamesMatch, hashFrames } from './common'; +import { LOCAL_VARIABLES_KEY } from './common'; +import { functionNamesMatch } from './common'; // This string is a placeholder that gets overwritten with the worker code. export const base64WorkerScript = '###LocalVariablesWorkerScript###'; @@ -20,23 +21,7 @@ function log(...args: unknown[]): void { export const localVariablesAsyncIntegration = defineIntegration((( integrationOptions: LocalVariablesIntegrationOptions = {}, ) => { - const cachedFrames: LRUMap = new LRUMap(20); - - function addLocalVariablesToException(exception: Exception): void { - const hash = hashFrames(exception?.stacktrace?.frames); - - if (hash === undefined) { - return; - } - - // Check if we have local variables for an exception that matches the hash - // remove is identical to get but also removes the entry from the cache - const cachedFrame = cachedFrames.remove(hash); - - if (cachedFrame === undefined) { - return; - } - + function addLocalVariablesToException(exception: Exception, localVariables: FrameVariables[]): void { // Filter out frames where the function name is `new Promise` since these are in the error.stack frames // but do not appear in the debugger call frames const frames = (exception.stacktrace?.frames || []).filter(frame => frame.function !== 'new Promise'); @@ -45,32 +30,39 @@ export const localVariablesAsyncIntegration = defineIntegration((( // Sentry frames are in reverse order const frameIndex = frames.length - i - 1; - const cachedFrameVariable = cachedFrame[i]; - const frameVariable = frames[frameIndex]; + const frameLocalVariables = localVariables[i]; + const frame = frames[frameIndex]; - if (!frameVariable || !cachedFrameVariable) { + if (!frame || !frameLocalVariables) { // Drop out if we run out of frames to match up break; } if ( // We need to have vars to add - cachedFrameVariable.vars === undefined || + frameLocalVariables.vars === undefined || // We're not interested in frames that are not in_app because the vars are not relevant - frameVariable.in_app === false || + frame.in_app === false || // The function names need to match - !functionNamesMatch(frameVariable.function, cachedFrameVariable.function) + !functionNamesMatch(frame.function, frameLocalVariables.function) ) { continue; } - frameVariable.vars = cachedFrameVariable.vars; + frame.vars = frameLocalVariables.vars; } } - function addLocalVariablesToEvent(event: Event): Event { - for (const exception of event.exception?.values || []) { - addLocalVariablesToException(exception); + function addLocalVariablesToEvent(event: Event, hint: EventHint): Event { + if ( + hint.originalException && + typeof hint.originalException === 'object' && + LOCAL_VARIABLES_KEY in hint.originalException && + Array.isArray(hint.originalException[LOCAL_VARIABLES_KEY]) + ) { + for (const exception of event.exception?.values || []) { + addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]); + } } return event; @@ -96,10 +88,6 @@ export const localVariablesAsyncIntegration = defineIntegration((( worker.terminate(); }); - worker.on('message', ({ exceptionHash, frames }) => { - cachedFrames.set(exceptionHash, frames); - }); - worker.once('error', (err: Error) => { log('Worker error', err); }); @@ -139,8 +127,8 @@ export const localVariablesAsyncIntegration = defineIntegration((( }, ); }, - processEvent(event: Event): Event { - return addLocalVariablesToEvent(event); + processEvent(event: Event, hint: EventHint): Event { + return addLocalVariablesToEvent(event, hint); }, }; }) satisfies IntegrationFn); diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index bf7aaa26cbf3..76be4e2334c9 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -1,7 +1,7 @@ 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'; +import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types'; +import { logger } from '@sentry/utils'; import { NODE_MAJOR } from '../../nodeVersion'; import type { NodeClient } from '../../sdk/client'; @@ -12,7 +12,8 @@ import type { RateLimitIncrement, Variables, } from './common'; -import { createRateLimiter, functionNamesMatch, hashFrames, hashFromStack } from './common'; +import { LOCAL_VARIABLES_KEY } from './common'; +import { createRateLimiter, functionNamesMatch } from './common'; type OnPauseEvent = InspectorNotification; export interface DebugSession { @@ -22,6 +23,8 @@ export interface DebugSession { setPauseOnExceptions(captureAll: boolean): void; /** Gets local variables for an objectId */ getLocalVariables(objectId: string, callback: (vars: Variables) => void): void; + /** Sets the local variables on the error object for later retrieval */ + setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void; } type Next = (result: T) => void; @@ -128,6 +131,13 @@ class AsyncSession implements DebugSession { }); } + public setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void { + this._session.post('Runtime.callFunctionOn', { + functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(localVariables)}; }`, + objectId, + }); + } + /** * Gets all the PropertyDescriptors of an object */ @@ -209,25 +219,10 @@ const _localVariablesSyncIntegration = (( options: LocalVariablesIntegrationOptions = {}, sessionOverride?: DebugSession, ) => { - const cachedFrames: LRUMap = new LRUMap(20); let rateLimiter: RateLimitIncrement | undefined; let shouldProcessEvent = false; - function addLocalVariablesToException(exception: Exception): void { - const hash = hashFrames(exception?.stacktrace?.frames); - - if (hash === undefined) { - return; - } - - // Check if we have local variables for an exception that matches the hash - // remove is identical to get but also removes the entry from the cache - const cachedFrame = cachedFrames.remove(hash); - - if (cachedFrame === undefined) { - return; - } - + function addLocalVariablesToException(exception: Exception, localVariables: FrameVariables[]): void { // Filter out frames where the function name is `new Promise` since these are in the error.stack frames // but do not appear in the debugger call frames const frames = (exception.stacktrace?.frames || []).filter(frame => frame.function !== 'new Promise'); @@ -236,32 +231,39 @@ const _localVariablesSyncIntegration = (( // Sentry frames are in reverse order const frameIndex = frames.length - i - 1; - const cachedFrameVariable = cachedFrame[i]; - const frameVariable = frames[frameIndex]; + const frameLocalVariables = localVariables[i]; + const frame = frames[frameIndex]; // Drop out if we run out of frames to match up - if (!frameVariable || !cachedFrameVariable) { + if (!frame || !frameLocalVariables) { break; } if ( // We need to have vars to add - cachedFrameVariable.vars === undefined || + frameLocalVariables.vars === undefined || // We're not interested in frames that are not in_app because the vars are not relevant - frameVariable.in_app === false || + frame.in_app === false || // The function names need to match - !functionNamesMatch(frameVariable.function, cachedFrameVariable.function) + !functionNamesMatch(frame.function, frameLocalVariables.function) ) { continue; } - frameVariable.vars = cachedFrameVariable.vars; + frame.vars = frameLocalVariables.vars; } } - function addLocalVariablesToEvent(event: Event): Event { - for (const exception of event?.exception?.values || []) { - addLocalVariablesToException(exception); + function addLocalVariablesToEvent(event: Event, hint: EventHint): Event { + if ( + hint.originalException && + typeof hint.originalException === 'object' && + LOCAL_VARIABLES_KEY in hint.originalException && + Array.isArray(hint.originalException[LOCAL_VARIABLES_KEY]) + ) { + for (const exception of event.exception?.values || []) { + addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]); + } } return event; @@ -289,7 +291,6 @@ const _localVariablesSyncIntegration = (( AsyncSession.create(sessionOverride).then( session => { function handlePaused( - stackParser: StackParser, { params: { reason, data, callFrames } }: InspectorNotification, complete: () => void, ): void { @@ -300,16 +301,15 @@ const _localVariablesSyncIntegration = (( rateLimiter?.(); - // data.description contains the original error.stack - const exceptionHash = hashFromStack(stackParser, data?.description); + const objectId = data?.objectId; - if (exceptionHash == undefined) { + if (objectId == undefined) { complete(); return; } const { add, next } = createCallbackList(frames => { - cachedFrames.set(exceptionHash, frames); + session.setLocalVarsOnError(objectId, frames); complete(); }); @@ -347,8 +347,7 @@ const _localVariablesSyncIntegration = (( const captureAll = options.captureAllExceptions !== false; session.configureAndConnect( - (ev, complete) => - handlePaused(clientOptions.stackParser, ev as InspectorNotification, complete), + (ev, complete) => handlePaused(ev as InspectorNotification, complete), captureAll, ); @@ -377,20 +376,13 @@ const _localVariablesSyncIntegration = (( }, ); }, - processEvent(event: Event): Event { + processEvent(event: Event, hint: EventHint): Event { if (shouldProcessEvent) { - return addLocalVariablesToEvent(event); + return addLocalVariablesToEvent(event, hint); } return event; }, - // These are entirely for testing - _getCachedFramesCount(): number { - return cachedFrames.size; - }, - _getFirstCachedFrame(): FrameVariables[] | undefined { - return cachedFrames.values()[0]; - }, }; }) satisfies IntegrationFn; diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts index 5bee22a84c29..2d2d8548f499 100644 --- a/packages/node/src/integrations/local-variables/worker.ts +++ b/packages/node/src/integrations/local-variables/worker.ts @@ -1,16 +1,12 @@ import type { Debugger, InspectorNotification, Runtime } from 'node:inspector'; import { Session } from 'node:inspector/promises'; import { parentPort, workerData } from 'node:worker_threads'; -import type { StackParser } from '@sentry/types'; -import { createStackParser, nodeStackLineParser } from '@sentry/utils'; -import { createGetModuleFromFilename } from '../../utils/module'; import type { LocalVariablesWorkerArgs, PausedExceptionEvent, RateLimitIncrement, Variables } from './common'; -import { createRateLimiter, hashFromStack } from './common'; +import { LOCAL_VARIABLES_KEY } from './common'; +import { createRateLimiter } from './common'; const options: LocalVariablesWorkerArgs = workerData; -const stackParser = createStackParser(nodeStackLineParser(createGetModuleFromFilename(options.basePath))); - function log(...args: unknown[]): void { if (options.debug) { // eslint-disable-next-line no-console @@ -86,11 +82,7 @@ async function getLocalVariables(session: Session, objectId: string): Promise { +async function handlePaused(session: Session, { reason, data, callFrames }: PausedExceptionEvent): Promise { if (reason !== 'exception' && reason !== 'promiseRejection') { return; } @@ -98,9 +90,9 @@ async function handlePaused( rateLimiter?.(); // data.description contains the original error.stack - const exceptionHash = hashFromStack(stackParser, data?.description); + const objectId = data?.objectId; - if (exceptionHash == undefined) { + if (objectId == undefined) { return; } @@ -123,7 +115,12 @@ async function handlePaused( } } - parentPort?.postMessage({ exceptionHash, frames }); + await session.post('Runtime.callFunctionOn', { + functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(frames)}; }`, + objectId, + }); + + parentPort?.postMessage({ objectId, frames }); } async function startDebugger(): Promise { @@ -141,7 +138,7 @@ async function startDebugger(): Promise { session.on('Debugger.paused', (event: InspectorNotification) => { isPaused = true; - handlePaused(session, stackParser, event.params as PausedExceptionEvent).then( + handlePaused(session, event.params as PausedExceptionEvent).then( () => { // After the pause work is complete, resume execution! return isPaused ? session.post('Debugger.resume') : Promise.resolve(); From 7ac938fdd698de2bf0881e5010a99cfffb4c1182 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 28 Aug 2024 13:31:43 +0100 Subject: [PATCH 2/8] increase memory usage test --- .../suites/public-api/LocalVariables/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 2640ecf94461..e2727ac8d6c5 100644 --- a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -94,8 +94,8 @@ conditionalTest({ min: 18 })('LocalVariables integration', () => { child.on('message', msg => { reportedCount++; const rssMb = (msg as { memUsage: { rss: number } }).memUsage.rss / 1024 / 1024; - // We shouldn't use more than 135MB of memory - expect(rssMb).toBeLessThan(135); + // We shouldn't use more than 140MB of memory + expect(rssMb).toBeLessThan(140); }); // Wait for 20 seconds From 46a057e89bd603dc4ff2b3781c9181b2ec1eb553 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 28 Aug 2024 13:34:16 +0100 Subject: [PATCH 3/8] Revert "increase memory usage test" This reverts commit 7ac938fdd698de2bf0881e5010a99cfffb4c1182. --- .../suites/public-api/LocalVariables/test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index e2727ac8d6c5..2640ecf94461 100644 --- a/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts +++ b/dev-packages/node-integration-tests/suites/public-api/LocalVariables/test.ts @@ -94,8 +94,8 @@ conditionalTest({ min: 18 })('LocalVariables integration', () => { child.on('message', msg => { reportedCount++; const rssMb = (msg as { memUsage: { rss: number } }).memUsage.rss / 1024 / 1024; - // We shouldn't use more than 140MB of memory - expect(rssMb).toBeLessThan(140); + // We shouldn't use more than 135MB of memory + expect(rssMb).toBeLessThan(135); }); // Wait for 20 seconds From 87d5d75d83030e4b5531907dae867ea897ffefd3 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 28 Aug 2024 18:50:23 +0100 Subject: [PATCH 4/8] Fix async memory leak --- .../local-variables/inspector.d.ts | 4 ++++ .../local-variables/local-variables-async.ts | 2 ++ .../local-variables/local-variables-sync.ts | 3 +++ .../integrations/local-variables/worker.ts | 20 ++++++++++++++----- 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/packages/node/src/integrations/local-variables/inspector.d.ts b/packages/node/src/integrations/local-variables/inspector.d.ts index 03d9652b2670..5cfd496f7626 100644 --- a/packages/node/src/integrations/local-variables/inspector.d.ts +++ b/packages/node/src/integrations/local-variables/inspector.d.ts @@ -24,6 +24,10 @@ declare module 'node:inspector/promises' { method: 'Runtime.callFunctionOn', params: Runtime.CallFunctionOnParameterType, ): Promise; + public post( + method: 'Runtime.releaseObject', + params: Runtime.ReleaseObjectParameterType, + ): Promise; public on( event: 'Debugger.paused', diff --git a/packages/node/src/integrations/local-variables/local-variables-async.ts b/packages/node/src/integrations/local-variables/local-variables-async.ts index cb788ad572db..d46fa224019f 100644 --- a/packages/node/src/integrations/local-variables/local-variables-async.ts +++ b/packages/node/src/integrations/local-variables/local-variables-async.ts @@ -63,6 +63,8 @@ export const localVariablesAsyncIntegration = defineIntegration((( for (const exception of event.exception?.values || []) { addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]); } + + hint.originalException[LOCAL_VARIABLES_KEY] = undefined; } return event; diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index 76be4e2334c9..b36f5ffb8f1b 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -134,6 +134,7 @@ class AsyncSession implements DebugSession { public setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void { this._session.post('Runtime.callFunctionOn', { functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(localVariables)}; }`, + silent: true, objectId, }); } @@ -264,6 +265,8 @@ const _localVariablesSyncIntegration = (( for (const exception of event.exception?.values || []) { addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]); } + + hint.originalException[LOCAL_VARIABLES_KEY] = undefined; } return event; diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts index 2d2d8548f499..b1339bc441d0 100644 --- a/packages/node/src/integrations/local-variables/worker.ts +++ b/packages/node/src/integrations/local-variables/worker.ts @@ -1,6 +1,6 @@ import type { Debugger, InspectorNotification, Runtime } from 'node:inspector'; import { Session } from 'node:inspector/promises'; -import { parentPort, workerData } from 'node:worker_threads'; +import { workerData } from 'node:worker_threads'; import type { LocalVariablesWorkerArgs, PausedExceptionEvent, RateLimitIncrement, Variables } from './common'; import { LOCAL_VARIABLES_KEY } from './common'; import { createRateLimiter } from './common'; @@ -82,7 +82,10 @@ async function getLocalVariables(session: Session, objectId: string): Promise { +async function handlePaused( + session: Session, + { reason, data, callFrames }: PausedExceptionEvent, +): Promise { if (reason !== 'exception' && reason !== 'promiseRejection') { return; } @@ -117,10 +120,11 @@ async function handlePaused(session: Session, { reason, data, callFrames }: Paus await session.post('Runtime.callFunctionOn', { functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(frames)}; }`, + silent: true, objectId, }); - parentPort?.postMessage({ objectId, frames }); + return objectId; } async function startDebugger(): Promise { @@ -139,9 +143,15 @@ async function startDebugger(): Promise { isPaused = true; handlePaused(session, event.params as PausedExceptionEvent).then( - () => { + async objectId => { // After the pause work is complete, resume execution! - return isPaused ? session.post('Debugger.resume') : Promise.resolve(); + if (isPaused) { + await session.post('Debugger.resume'); + } + + if (objectId) { + return session.post('Runtime.releaseObject', { objectId }); + } }, _ => { // ignore From efacd0a64da3b9b3cedd7ead856f19236007a5b6 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 29 Aug 2024 16:47:56 +0100 Subject: [PATCH 5/8] Fix cleanup --- .../integrations/local-variables/worker.ts | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts index b1339bc441d0..5ae000ef8b6a 100644 --- a/packages/node/src/integrations/local-variables/worker.ts +++ b/packages/node/src/integrations/local-variables/worker.ts @@ -84,18 +84,15 @@ let rateLimiter: RateLimitIncrement | undefined; async function handlePaused( session: Session, - { reason, data, callFrames }: PausedExceptionEvent, -): Promise { + { reason, data: { objectId: errorObjectId }, callFrames }: PausedExceptionEvent, +): Promise { if (reason !== 'exception' && reason !== 'promiseRejection') { return; } rateLimiter?.(); - // data.description contains the original error.stack - const objectId = data?.objectId; - - if (objectId == undefined) { + if (errorObjectId == undefined) { return; } @@ -118,13 +115,20 @@ async function handlePaused( } } + // We write the local variables to a property on the error object. These can be read by the integration as the error + // event pass through the SDK event pipeline await session.post('Runtime.callFunctionOn', { functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(frames)}; }`, silent: true, - objectId, + objectId: errorObjectId, }); - return objectId; + // We need to cleanup this object but we need to leave enough time for our changes to be picked up in the main app + // context + setTimeout(() => { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + session.post('Runtime.releaseObject', { objectId: errorObjectId }); + }, 10_000); } async function startDebugger(): Promise { @@ -143,15 +147,11 @@ async function startDebugger(): Promise { isPaused = true; handlePaused(session, event.params as PausedExceptionEvent).then( - async objectId => { + async () => { // After the pause work is complete, resume execution! if (isPaused) { await session.post('Debugger.resume'); } - - if (objectId) { - return session.post('Runtime.releaseObject', { objectId }); - } }, _ => { // ignore From 069d6f25f69eafcf68d725e799d0fda769886625 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 4 Sep 2024 10:38:01 +0200 Subject: [PATCH 6/8] Fix async --- .../integrations/local-variables/worker.ts | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts index 5ae000ef8b6a..88e80b9999cf 100644 --- a/packages/node/src/integrations/local-variables/worker.ts +++ b/packages/node/src/integrations/local-variables/worker.ts @@ -84,15 +84,15 @@ let rateLimiter: RateLimitIncrement | undefined; async function handlePaused( session: Session, - { reason, data: { objectId: errorObjectId }, callFrames }: PausedExceptionEvent, -): Promise { + { reason, data: { objectId }, callFrames }: PausedExceptionEvent, +): Promise { if (reason !== 'exception' && reason !== 'promiseRejection') { return; } rateLimiter?.(); - if (errorObjectId == undefined) { + if (objectId == undefined) { return; } @@ -120,15 +120,10 @@ async function handlePaused( await session.post('Runtime.callFunctionOn', { functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(frames)}; }`, silent: true, - objectId: errorObjectId, + objectId, }); - // We need to cleanup this object but we need to leave enough time for our changes to be picked up in the main app - // context - setTimeout(() => { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - session.post('Runtime.releaseObject', { objectId: errorObjectId }); - }, 10_000); + return objectId; } async function startDebugger(): Promise { @@ -147,14 +142,22 @@ async function startDebugger(): Promise { isPaused = true; handlePaused(session, event.params as PausedExceptionEvent).then( - async () => { + async objectId => { // After the pause work is complete, resume execution! if (isPaused) { await session.post('Debugger.resume'); } + + if (objectId) { + // setImmediate ensures that the debugger has resumed before we release the error object. + // Without this, we get a memory leak + setImmediate(async () => { + await session.post('Runtime.releaseObject', { objectId }); + }); + } }, _ => { - // ignore + // ignore any errors }, ); }); From ac543cfa1ff4e958be86466055fcbcdca0c4ca90 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 4 Sep 2024 10:49:48 +0200 Subject: [PATCH 7/8] Revert sync debugger changes --- .../local-variables/local-variables-sync.ts | 105 +++++++++++------- 1 file changed, 66 insertions(+), 39 deletions(-) diff --git a/packages/node/src/integrations/local-variables/local-variables-sync.ts b/packages/node/src/integrations/local-variables/local-variables-sync.ts index b36f5ffb8f1b..d3203614330f 100644 --- a/packages/node/src/integrations/local-variables/local-variables-sync.ts +++ b/packages/node/src/integrations/local-variables/local-variables-sync.ts @@ -1,7 +1,7 @@ import type { Debugger, InspectorNotification, Runtime, Session } from 'node:inspector'; import { defineIntegration, getClient } from '@sentry/core'; -import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import type { Event, Exception, IntegrationFn, StackFrame, StackParser } from '@sentry/types'; +import { LRUMap, logger } from '@sentry/utils'; import { NODE_MAJOR } from '../../nodeVersion'; import type { NodeClient } from '../../sdk/client'; @@ -12,9 +12,30 @@ import type { RateLimitIncrement, Variables, } from './common'; -import { LOCAL_VARIABLES_KEY } from './common'; import { createRateLimiter, functionNamesMatch } from './common'; +/** Creates a unique hash from stack frames */ +export function hashFrames(frames: StackFrame[] | undefined): string | undefined { + if (frames === undefined) { + return; + } + + // Only hash the 10 most recent frames (ie. the last 10) + return frames.slice(-10).reduce((acc, frame) => `${acc},${frame.function},${frame.lineno},${frame.colno}`, ''); +} + +/** + * We use the stack parser to create a unique hash from the exception stack trace + * This is used to lookup vars when the exception passes through the event processor + */ +export function hashFromStack(stackParser: StackParser, stack: string | undefined): string | undefined { + if (stack === undefined) { + return undefined; + } + + return hashFrames(stackParser(stack, 1)); +} + type OnPauseEvent = InspectorNotification; export interface DebugSession { /** Configures and connects to the debug session */ @@ -23,8 +44,6 @@ export interface DebugSession { setPauseOnExceptions(captureAll: boolean): void; /** Gets local variables for an objectId */ getLocalVariables(objectId: string, callback: (vars: Variables) => void): void; - /** Sets the local variables on the error object for later retrieval */ - setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void; } type Next = (result: T) => void; @@ -131,14 +150,6 @@ class AsyncSession implements DebugSession { }); } - public setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void { - this._session.post('Runtime.callFunctionOn', { - functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(localVariables)}; }`, - silent: true, - objectId, - }); - } - /** * Gets all the PropertyDescriptors of an object */ @@ -220,10 +231,25 @@ const _localVariablesSyncIntegration = (( options: LocalVariablesIntegrationOptions = {}, sessionOverride?: DebugSession, ) => { + const cachedFrames: LRUMap = new LRUMap(20); let rateLimiter: RateLimitIncrement | undefined; let shouldProcessEvent = false; - function addLocalVariablesToException(exception: Exception, localVariables: FrameVariables[]): void { + function addLocalVariablesToException(exception: Exception): void { + const hash = hashFrames(exception?.stacktrace?.frames); + + if (hash === undefined) { + return; + } + + // Check if we have local variables for an exception that matches the hash + // remove is identical to get but also removes the entry from the cache + const cachedFrame = cachedFrames.remove(hash); + + if (cachedFrame === undefined) { + return; + } + // Filter out frames where the function name is `new Promise` since these are in the error.stack frames // but do not appear in the debugger call frames const frames = (exception.stacktrace?.frames || []).filter(frame => frame.function !== 'new Promise'); @@ -232,41 +258,32 @@ const _localVariablesSyncIntegration = (( // Sentry frames are in reverse order const frameIndex = frames.length - i - 1; - const frameLocalVariables = localVariables[i]; - const frame = frames[frameIndex]; + const cachedFrameVariable = cachedFrame[i]; + const frameVariable = frames[frameIndex]; // Drop out if we run out of frames to match up - if (!frame || !frameLocalVariables) { + if (!frameVariable || !cachedFrameVariable) { break; } if ( // We need to have vars to add - frameLocalVariables.vars === undefined || + cachedFrameVariable.vars === undefined || // We're not interested in frames that are not in_app because the vars are not relevant - frame.in_app === false || + frameVariable.in_app === false || // The function names need to match - !functionNamesMatch(frame.function, frameLocalVariables.function) + !functionNamesMatch(frameVariable.function, cachedFrameVariable.function) ) { continue; } - frame.vars = frameLocalVariables.vars; + frameVariable.vars = cachedFrameVariable.vars; } } - function addLocalVariablesToEvent(event: Event, hint: EventHint): Event { - if ( - hint.originalException && - typeof hint.originalException === 'object' && - LOCAL_VARIABLES_KEY in hint.originalException && - Array.isArray(hint.originalException[LOCAL_VARIABLES_KEY]) - ) { - for (const exception of event.exception?.values || []) { - addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]); - } - - hint.originalException[LOCAL_VARIABLES_KEY] = undefined; + function addLocalVariablesToEvent(event: Event): Event { + for (const exception of event?.exception?.values || []) { + addLocalVariablesToException(exception); } return event; @@ -294,6 +311,7 @@ const _localVariablesSyncIntegration = (( AsyncSession.create(sessionOverride).then( session => { function handlePaused( + stackParser: StackParser, { params: { reason, data, callFrames } }: InspectorNotification, complete: () => void, ): void { @@ -304,15 +322,16 @@ const _localVariablesSyncIntegration = (( rateLimiter?.(); - const objectId = data?.objectId; + // data.description contains the original error.stack + const exceptionHash = hashFromStack(stackParser, data?.description); - if (objectId == undefined) { + if (exceptionHash == undefined) { complete(); return; } const { add, next } = createCallbackList(frames => { - session.setLocalVarsOnError(objectId, frames); + cachedFrames.set(exceptionHash, frames); complete(); }); @@ -350,7 +369,8 @@ const _localVariablesSyncIntegration = (( const captureAll = options.captureAllExceptions !== false; session.configureAndConnect( - (ev, complete) => handlePaused(ev as InspectorNotification, complete), + (ev, complete) => + handlePaused(clientOptions.stackParser, ev as InspectorNotification, complete), captureAll, ); @@ -379,13 +399,20 @@ const _localVariablesSyncIntegration = (( }, ); }, - processEvent(event: Event, hint: EventHint): Event { + processEvent(event: Event): Event { if (shouldProcessEvent) { - return addLocalVariablesToEvent(event, hint); + return addLocalVariablesToEvent(event); } return event; }, + // These are entirely for testing + _getCachedFramesCount(): number { + return cachedFrames.size; + }, + _getFirstCachedFrame(): FrameVariables[] | undefined { + return cachedFrames.values()[0]; + }, }; }) satisfies IntegrationFn; From efbad103e41d40744f25f00e0f8b6bdd541b2f20 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 4 Sep 2024 10:59:09 +0200 Subject: [PATCH 8/8] Fix node v22 --- packages/node/src/integrations/local-variables/worker.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/node/src/integrations/local-variables/worker.ts b/packages/node/src/integrations/local-variables/worker.ts index 88e80b9999cf..eb4fee87947c 100644 --- a/packages/node/src/integrations/local-variables/worker.ts +++ b/packages/node/src/integrations/local-variables/worker.ts @@ -149,11 +149,11 @@ async function startDebugger(): Promise { } if (objectId) { - // setImmediate ensures that the debugger has resumed before we release the error object. - // Without this, we get a memory leak - setImmediate(async () => { + // The object must be released after the debugger has resumed or we get a memory leak. + // For node v20, setImmediate is enough here but for v22 a longer delay is required + setTimeout(async () => { await session.post('Runtime.releaseObject', { objectId }); - }); + }, 1_000); } }, _ => {