Skip to content

Commit

Permalink
fix(node): Remove ambiguity and race conditions when matching local v…
Browse files Browse the repository at this point in the history
…ariables to exceptions (#13501)

Closes #13415

This PR only modifies the async version of this integration which is
used for Node > v19. I tried applying similar changes to the sync
integration and I cannot get it to work without causing memory leaks.

@Bruno-DaSilva has been helping me explore different ways to fix a few
fundamental issues with the local variables integration. Bruno found a
way to [write to the error
object](#13415 (comment))
from the debugger which removes any ambiguity over which variables go
with which exception.

This allows us to remove the stack parsing and hashing which we were
using previously to match up exceptions.

Rather than write the `objectId` to the error, I have used this to write
the entire local variables array directly to the error object. This
completely negates the need to post the local variables from the worker
thread which removes any possibility of race conditions. We then later
pull the local variables directly from
`hint.originalException.__SENTRY_ERROR_LOCAL_VARIABLES__`.
  • Loading branch information
timfish authored Sep 10, 2024
1 parent 7fa366f commit 4c6dd80
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 80 deletions.
29 changes: 6 additions & 23 deletions packages/node/src/integrations/local-variables/common.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import type { Debugger } from 'node:inspector';
import type { StackFrame, StackParser } from '@sentry/types';

export type Variables = Record<string, unknown>;

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.
Expand Down Expand Up @@ -55,6 +59,7 @@ export type PausedExceptionEvent = Debugger.PausedEventDataType & {
data: {
// This contains error.stack
description: string;
objectId?: string;
};
};

Expand All @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions packages/node/src/integrations/local-variables/inspector.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ declare module 'node:inspector/promises' {
method: 'Runtime.getProperties',
params: Runtime.GetPropertiesParameterType,
): Promise<Runtime.GetPropertiesReturnType>;
public post(
method: 'Runtime.callFunctionOn',
params: Runtime.CallFunctionOnParameterType,
): Promise<Runtime.CallFunctionOnReturnType>;
public post(
method: 'Runtime.releaseObject',
params: Runtime.ReleaseObjectParameterType,
): Promise<Runtime.ReleaseObjectReturnType>;

public on(
event: 'Debugger.paused',
Expand Down
Original file line number Diff line number Diff line change
@@ -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###';
Expand All @@ -20,23 +21,7 @@ function log(...args: unknown[]): void {
export const localVariablesAsyncIntegration = defineIntegration(((
integrationOptions: LocalVariablesIntegrationOptions = {},
) => {
const cachedFrames: LRUMap<string, FrameVariables[]> = 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');
Expand All @@ -45,32 +30,41 @@ 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]);
}

hint.originalException[LOCAL_VARIABLES_KEY] = undefined;
}

return event;
Expand All @@ -96,10 +90,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);
});
Expand Down Expand Up @@ -139,8 +129,8 @@ export const localVariablesAsyncIntegration = defineIntegration(((
},
);
},
processEvent(event: Event): Event {
return addLocalVariablesToEvent(event);
processEvent(event: Event, hint: EventHint): Event {
return addLocalVariablesToEvent(event, hint);
},
};
}) satisfies IntegrationFn);
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
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 type { Event, Exception, IntegrationFn, StackFrame, StackParser } from '@sentry/types';
import { LRUMap, logger } from '@sentry/utils';

import { NODE_MAJOR } from '../../nodeVersion';
Expand All @@ -12,7 +12,29 @@ import type {
RateLimitIncrement,
Variables,
} from './common';
import { createRateLimiter, functionNamesMatch, hashFrames, hashFromStack } 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<Debugger.PausedEventDataType>;
export interface DebugSession {
Expand Down
48 changes: 29 additions & 19 deletions packages/node/src/integrations/local-variables/worker.ts
Original file line number Diff line number Diff line change
@@ -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 { workerData } from 'node:worker_threads';
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
Expand Down Expand Up @@ -88,19 +84,15 @@ let rateLimiter: RateLimitIncrement | undefined;

async function handlePaused(
session: Session,
stackParser: StackParser,
{ reason, data, callFrames }: PausedExceptionEvent,
): Promise<void> {
{ reason, data: { objectId }, callFrames }: PausedExceptionEvent,
): Promise<string | undefined> {
if (reason !== 'exception' && reason !== 'promiseRejection') {
return;
}

rateLimiter?.();

// data.description contains the original error.stack
const exceptionHash = hashFromStack(stackParser, data?.description);

if (exceptionHash == undefined) {
if (objectId == undefined) {
return;
}

Expand All @@ -123,7 +115,15 @@ async function handlePaused(
}
}

parentPort?.postMessage({ exceptionHash, frames });
// 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,
});

return objectId;
}

async function startDebugger(): Promise<void> {
Expand All @@ -141,13 +141,23 @@ async function startDebugger(): Promise<void> {
session.on('Debugger.paused', (event: InspectorNotification<Debugger.PausedEventDataType>) => {
isPaused = true;

handlePaused(session, stackParser, event.params as PausedExceptionEvent).then(
() => {
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) {
// 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);
}
},
_ => {
// ignore
// ignore any errors
},
);
});
Expand Down

0 comments on commit 4c6dd80

Please sign in to comment.