Skip to content

Commit d6d0462

Browse files
timfishtimfish
committed
fix(node): Remove race conditions and ambiguity when matching local variables
1 parent 3158c21 commit d6d0462

File tree

5 files changed

+83
-119
lines changed

5 files changed

+83
-119
lines changed

‎packages/node/src/integrations/local-variables/common.ts

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
import type { Debugger } from 'node:inspector';
2-
import type { StackFrame, StackParser } from '@sentry/types';
32

43
export type Variables = Record<string, unknown>;
54

65
export type RateLimitIncrement = () => void;
76

7+
/**
8+
* The key used to store the local variables on the error object.
9+
*/
10+
export const LOCAL_VARIABLES_KEY = '__SENTRY_ERROR_LOCAL_VARIABLES__';
11+
812
/**
913
* Creates a rate limiter that will call the disable callback when the rate limit is reached and the enable callback
1014
* when a timeout has occurred.
@@ -55,6 +59,7 @@ export type PausedExceptionEvent = Debugger.PausedEventDataType & {
5559
data: {
5660
// This contains error.stack
5761
description: string;
62+
objectId?: string;
5863
};
5964
};
6065

@@ -68,28 +73,6 @@ export function functionNamesMatch(a: string | undefined, b: string | undefined)
6873
return a === b || (isAnonymous(a) && isAnonymous(b));
6974
}
7075

71-
/** Creates a unique hash from stack frames */
72-
export function hashFrames(frames: StackFrame[] | undefined): string | undefined {
73-
if (frames === undefined) {
74-
return;
75-
}
76-
77-
// Only hash the 10 most recent frames (ie. the last 10)
78-
return frames.slice(-10).reduce((acc, frame) => `${acc},${frame.function},${frame.lineno},${frame.colno}`, '');
79-
}
80-
81-
/**
82-
* We use the stack parser to create a unique hash from the exception stack trace
83-
* This is used to lookup vars when the exception passes through the event processor
84-
*/
85-
export function hashFromStack(stackParser: StackParser, stack: string | undefined): string | undefined {
86-
if (stack === undefined) {
87-
return undefined;
88-
}
89-
90-
return hashFrames(stackParser(stack, 1));
91-
}
92-
9376
export interface FrameVariables {
9477
function: string;
9578
vars?: Variables;

‎packages/node/src/integrations/local-variables/inspector.d.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ declare module 'node:inspector/promises' {
2020
method: 'Runtime.getProperties',
2121
params: Runtime.GetPropertiesParameterType,
2222
): Promise<Runtime.GetPropertiesReturnType>;
23+
public post(
24+
method: 'Runtime.callFunctionOn',
25+
params: Runtime.CallFunctionOnParameterType,
26+
): Promise<Runtime.CallFunctionOnReturnType>;
2327

2428
public on(
2529
event: 'Debugger.paused',

‎packages/node/src/integrations/local-variables/local-variables-async.ts

Lines changed: 24 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import { Worker } from 'node:worker_threads';
22
import { defineIntegration } from '@sentry/core';
3-
import type { Event, Exception, IntegrationFn } from '@sentry/types';
4-
import { LRUMap, logger } from '@sentry/utils';
3+
import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types';
4+
import { logger } from '@sentry/utils';
55

66
import type { NodeClient } from '../../sdk/client';
77
import type { FrameVariables, LocalVariablesIntegrationOptions, LocalVariablesWorkerArgs } from './common';
8-
import { functionNamesMatch, hashFrames } from './common';
8+
import { LOCAL_VARIABLES_KEY } from './common';
9+
import { functionNamesMatch } from './common';
910

1011
// This string is a placeholder that gets overwritten with the worker code.
1112
export const base64WorkerScript = '###LocalVariablesWorkerScript###';
@@ -20,23 +21,7 @@ function log(...args: unknown[]): void {
2021
export const localVariablesAsyncIntegration = defineIntegration(((
2122
integrationOptions: LocalVariablesIntegrationOptions = {},
2223
) => {
23-
const cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
24-
25-
function addLocalVariablesToException(exception: Exception): void {
26-
const hash = hashFrames(exception?.stacktrace?.frames);
27-
28-
if (hash === undefined) {
29-
return;
30-
}
31-
32-
// Check if we have local variables for an exception that matches the hash
33-
// remove is identical to get but also removes the entry from the cache
34-
const cachedFrame = cachedFrames.remove(hash);
35-
36-
if (cachedFrame === undefined) {
37-
return;
38-
}
39-
24+
function addLocalVariablesToException(exception: Exception, localVariables: FrameVariables[]): void {
4025
// Filter out frames where the function name is `new Promise` since these are in the error.stack frames
4126
// but do not appear in the debugger call frames
4227
const frames = (exception.stacktrace?.frames || []).filter(frame => frame.function !== 'new Promise');
@@ -45,32 +30,39 @@ export const localVariablesAsyncIntegration = defineIntegration(((
4530
// Sentry frames are in reverse order
4631
const frameIndex = frames.length - i - 1;
4732

48-
const cachedFrameVariable = cachedFrame[i];
49-
const frameVariable = frames[frameIndex];
33+
const frameLocalVariables = localVariables[i];
34+
const frame = frames[frameIndex];
5035

51-
if (!frameVariable || !cachedFrameVariable) {
36+
if (!frame || !frameLocalVariables) {
5237
// Drop out if we run out of frames to match up
5338
break;
5439
}
5540

5641
if (
5742
// We need to have vars to add
58-
cachedFrameVariable.vars === undefined ||
43+
frameLocalVariables.vars === undefined ||
5944
// We're not interested in frames that are not in_app because the vars are not relevant
60-
frameVariable.in_app === false ||
45+
frame.in_app === false ||
6146
// The function names need to match
62-
!functionNamesMatch(frameVariable.function, cachedFrameVariable.function)
47+
!functionNamesMatch(frame.function, frameLocalVariables.function)
6348
) {
6449
continue;
6550
}
6651

67-
frameVariable.vars = cachedFrameVariable.vars;
52+
frame.vars = frameLocalVariables.vars;
6853
}
6954
}
7055

71-
function addLocalVariablesToEvent(event: Event): Event {
72-
for (const exception of event.exception?.values || []) {
73-
addLocalVariablesToException(exception);
56+
function addLocalVariablesToEvent(event: Event, hint: EventHint): Event {
57+
if (
58+
hint.originalException &&
59+
typeof hint.originalException === 'object' &&
60+
LOCAL_VARIABLES_KEY in hint.originalException &&
61+
Array.isArray(hint.originalException[LOCAL_VARIABLES_KEY])
62+
) {
63+
for (const exception of event.exception?.values || []) {
64+
addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]);
65+
}
7466
}
7567

7668
return event;
@@ -96,10 +88,6 @@ export const localVariablesAsyncIntegration = defineIntegration(((
9688
worker.terminate();
9789
});
9890

99-
worker.on('message', ({ exceptionHash, frames }) => {
100-
cachedFrames.set(exceptionHash, frames);
101-
});
102-
10391
worker.once('error', (err: Error) => {
10492
log('Worker error', err);
10593
});
@@ -139,8 +127,8 @@ export const localVariablesAsyncIntegration = defineIntegration(((
139127
},
140128
);
141129
},
142-
processEvent(event: Event): Event {
143-
return addLocalVariablesToEvent(event);
130+
processEvent(event: Event, hint: EventHint): Event {
131+
return addLocalVariablesToEvent(event, hint);
144132
},
145133
};
146134
}) satisfies IntegrationFn);

‎packages/node/src/integrations/local-variables/local-variables-sync.ts

Lines changed: 37 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { Debugger, InspectorNotification, Runtime, Session } from 'node:inspector';
22
import { defineIntegration, getClient } from '@sentry/core';
3-
import type { Event, Exception, IntegrationFn, StackParser } from '@sentry/types';
4-
import { LRUMap, logger } from '@sentry/utils';
3+
import type { Event, EventHint, Exception, IntegrationFn } from '@sentry/types';
4+
import { logger } from '@sentry/utils';
55

66
import { NODE_MAJOR } from '../../nodeVersion';
77
import type { NodeClient } from '../../sdk/client';
@@ -12,7 +12,8 @@ import type {
1212
RateLimitIncrement,
1313
Variables,
1414
} from './common';
15-
import { createRateLimiter, functionNamesMatch, hashFrames, hashFromStack } from './common';
15+
import { LOCAL_VARIABLES_KEY } from './common';
16+
import { createRateLimiter, functionNamesMatch } from './common';
1617

1718
type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
1819
export interface DebugSession {
@@ -22,6 +23,8 @@ export interface DebugSession {
2223
setPauseOnExceptions(captureAll: boolean): void;
2324
/** Gets local variables for an objectId */
2425
getLocalVariables(objectId: string, callback: (vars: Variables) => void): void;
26+
/** Sets the local variables on the error object for later retrieval */
27+
setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void;
2528
}
2629

2730
type Next<T> = (result: T) => void;
@@ -128,6 +131,13 @@ class AsyncSession implements DebugSession {
128131
});
129132
}
130133

134+
public setLocalVarsOnError(objectId: string, localVariables: FrameVariables[]): void {
135+
this._session.post('Runtime.callFunctionOn', {
136+
functionDeclaration: `function() { this.${LOCAL_VARIABLES_KEY} = ${JSON.stringify(localVariables)}; }`,
137+
objectId,
138+
});
139+
}
140+
131141
/**
132142
* Gets all the PropertyDescriptors of an object
133143
*/
@@ -209,25 +219,10 @@ const _localVariablesSyncIntegration = ((
209219
options: LocalVariablesIntegrationOptions = {},
210220
sessionOverride?: DebugSession,
211221
) => {
212-
const cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
213222
let rateLimiter: RateLimitIncrement | undefined;
214223
let shouldProcessEvent = false;
215224

216-
function addLocalVariablesToException(exception: Exception): void {
217-
const hash = hashFrames(exception?.stacktrace?.frames);
218-
219-
if (hash === undefined) {
220-
return;
221-
}
222-
223-
// Check if we have local variables for an exception that matches the hash
224-
// remove is identical to get but also removes the entry from the cache
225-
const cachedFrame = cachedFrames.remove(hash);
226-
227-
if (cachedFrame === undefined) {
228-
return;
229-
}
230-
225+
function addLocalVariablesToException(exception: Exception, localVariables: FrameVariables[]): void {
231226
// Filter out frames where the function name is `new Promise` since these are in the error.stack frames
232227
// but do not appear in the debugger call frames
233228
const frames = (exception.stacktrace?.frames || []).filter(frame => frame.function !== 'new Promise');
@@ -236,32 +231,39 @@ const _localVariablesSyncIntegration = ((
236231
// Sentry frames are in reverse order
237232
const frameIndex = frames.length - i - 1;
238233

239-
const cachedFrameVariable = cachedFrame[i];
240-
const frameVariable = frames[frameIndex];
234+
const frameLocalVariables = localVariables[i];
235+
const frame = frames[frameIndex];
241236

242237
// Drop out if we run out of frames to match up
243-
if (!frameVariable || !cachedFrameVariable) {
238+
if (!frame || !frameLocalVariables) {
244239
break;
245240
}
246241

247242
if (
248243
// We need to have vars to add
249-
cachedFrameVariable.vars === undefined ||
244+
frameLocalVariables.vars === undefined ||
250245
// We're not interested in frames that are not in_app because the vars are not relevant
251-
frameVariable.in_app === false ||
246+
frame.in_app === false ||
252247
// The function names need to match
253-
!functionNamesMatch(frameVariable.function, cachedFrameVariable.function)
248+
!functionNamesMatch(frame.function, frameLocalVariables.function)
254249
) {
255250
continue;
256251
}
257252

258-
frameVariable.vars = cachedFrameVariable.vars;
253+
frame.vars = frameLocalVariables.vars;
259254
}
260255
}
261256

262-
function addLocalVariablesToEvent(event: Event): Event {
263-
for (const exception of event?.exception?.values || []) {
264-
addLocalVariablesToException(exception);
257+
function addLocalVariablesToEvent(event: Event, hint: EventHint): Event {
258+
if (
259+
hint.originalException &&
260+
typeof hint.originalException === 'object' &&
261+
LOCAL_VARIABLES_KEY in hint.originalException &&
262+
Array.isArray(hint.originalException[LOCAL_VARIABLES_KEY])
263+
) {
264+
for (const exception of event.exception?.values || []) {
265+
addLocalVariablesToException(exception, hint.originalException[LOCAL_VARIABLES_KEY]);
266+
}
265267
}
266268

267269
return event;
@@ -289,7 +291,6 @@ const _localVariablesSyncIntegration = ((
289291
AsyncSession.create(sessionOverride).then(
290292
session => {
291293
function handlePaused(
292-
stackParser: StackParser,
293294
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
294295
complete: () => void,
295296
): void {
@@ -300,16 +301,15 @@ const _localVariablesSyncIntegration = ((
300301

301302
rateLimiter?.();
302303

303-
// data.description contains the original error.stack
304-
const exceptionHash = hashFromStack(stackParser, data?.description);
304+
const objectId = data?.objectId;
305305

306-
if (exceptionHash == undefined) {
306+
if (objectId == undefined) {
307307
complete();
308308
return;
309309
}
310310

311311
const { add, next } = createCallbackList<FrameVariables[]>(frames => {
312-
cachedFrames.set(exceptionHash, frames);
312+
session.setLocalVarsOnError(objectId, frames);
313313
complete();
314314
});
315315

@@ -347,8 +347,7 @@ const _localVariablesSyncIntegration = ((
347347
const captureAll = options.captureAllExceptions !== false;
348348

349349
session.configureAndConnect(
350-
(ev, complete) =>
351-
handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
350+
(ev, complete) => handlePaused(ev as InspectorNotification<PausedExceptionEvent>, complete),
352351
captureAll,
353352
);
354353

@@ -377,20 +376,13 @@ const _localVariablesSyncIntegration = ((
377376
},
378377
);
379378
},
380-
processEvent(event: Event): Event {
379+
processEvent(event: Event, hint: EventHint): Event {
381380
if (shouldProcessEvent) {
382-
return addLocalVariablesToEvent(event);
381+
return addLocalVariablesToEvent(event, hint);
383382
}
384383

385384
return event;
386385
},
387-
// These are entirely for testing
388-
_getCachedFramesCount(): number {
389-
return cachedFrames.size;
390-
},
391-
_getFirstCachedFrame(): FrameVariables[] | undefined {
392-
return cachedFrames.values()[0];
393-
},
394386
};
395387
}) satisfies IntegrationFn;
396388

0 commit comments

Comments
 (0)