Skip to content

Commit 98bee7b

Browse files
JealousGxisaacs
authored andcommitted
fix(node-core): Fix race condition in local variables integration
The `LocalVariables` integrations `setupOnce` method was `async`, which violates the `Integration` interface. This caused a race condition where events could be processed before the integration was fully initialized, leading to missed local variables. This commit fixes the race condition by: - Making `setupOnce` synchronous to adhere to the interface contract. - Moving the asynchronous initialization logic to a separate `setup` function. - Making `processEvent` asynchronous and awaiting the result of the `setup` function, ensuring that the integration is fully initialized before processing any events. - Updating the tests to correctly `await` the `processEvent` method.
1 parent 5f8cb79 commit 98bee7b

File tree

2 files changed

+109
-101
lines changed

2 files changed

+109
-101
lines changed

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

Lines changed: 105 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -291,120 +291,128 @@ const _localVariablesSyncIntegration = ((
291291

292292
const testHelperMethods = localVariablesTestHelperMethods(cachedFrames);
293293

294-
return {
295-
name: INTEGRATION_NAME,
296-
async setupOnce() {
297-
const client = getClient<NodeClient>();
298-
const clientOptions = client?.getOptions();
299-
300-
if (!clientOptions?.includeLocalVariables) {
301-
return;
302-
}
294+
let setupPromise: Promise<void> | undefined;
303295

304-
// Only setup this integration if the Node version is >= v18
305-
// https://github.com/getsentry/sentry-javascript/issues/7697
306-
const unsupportedNodeVersion = NODE_MAJOR < 18;
307-
308-
if (unsupportedNodeVersion) {
309-
debug.log('The `LocalVariables` integration is only supported on Node >= v18.');
310-
return;
311-
}
312-
313-
if (await isDebuggerEnabled()) {
314-
debug.warn('Local variables capture has been disabled because the debugger was already enabled');
315-
return;
316-
}
296+
async function setup(): Promise<void> {
297+
const client = getClient<NodeClient>();
298+
const clientOptions = client?.getOptions();
317299

318-
try {
319-
const session = await AsyncSession.create(sessionOverride);
320-
321-
const handlePaused = (
322-
stackParser: StackParser,
323-
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
324-
complete: () => void,
325-
): void => {
326-
if (reason !== 'exception' && reason !== 'promiseRejection') {
327-
complete();
328-
return;
329-
}
300+
if (!clientOptions?.includeLocalVariables) {
301+
return;
302+
}
330303

331-
rateLimiter?.();
304+
// Only setup this integration if the Node version is >= v18
305+
// https://github.com/getsentry/sentry-javascript/issues/7697
306+
const unsupportedNodeVersion = NODE_MAJOR < 18;
332307

333-
// data.description contains the original error.stack
334-
const exceptionHash = hashFromStack(stackParser, data.description);
308+
if (unsupportedNodeVersion) {
309+
debug.log('The `LocalVariables` integration is only supported on Node >= v18.');
310+
return;
311+
}
335312

336-
if (exceptionHash == undefined) {
337-
complete();
338-
return;
339-
}
313+
if (await isDebuggerEnabled()) {
314+
debug.warn('Local variables capture has been disabled because the debugger was already enabled');
315+
return;
316+
}
340317

341-
const { add, next } = createCallbackList<FrameVariables[]>(frames => {
342-
cachedFrames.set(exceptionHash, frames);
343-
complete();
344-
});
318+
try {
319+
const session = await AsyncSession.create(sessionOverride);
320+
321+
const handlePaused = (
322+
stackParser: StackParser,
323+
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
324+
complete: () => void,
325+
): void => {
326+
if (reason !== 'exception' && reason !== 'promiseRejection') {
327+
complete();
328+
return;
329+
}
345330

346-
// Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
347-
// For this reason we only attempt to get local variables for the first 5 frames
348-
for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
349-
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
350-
const { scopeChain, functionName, this: obj } = callFrames[i]!;
331+
rateLimiter?.();
351332

352-
const localScope = scopeChain.find(scope => scope.type === 'local');
333+
// data.description contains the original error.stack
334+
const exceptionHash = hashFromStack(stackParser, data.description);
353335

354-
// obj.className is undefined in ESM modules
355-
const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;
336+
if (exceptionHash == undefined) {
337+
complete();
338+
return;
339+
}
356340

357-
if (localScope?.object.objectId === undefined) {
358-
add(frames => {
359-
frames[i] = { function: fn };
341+
const { add, next } = createCallbackList<FrameVariables[]>(frames => {
342+
cachedFrames.set(exceptionHash, frames);
343+
complete();
344+
});
345+
346+
// Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
347+
// For this reason we only attempt to get local variables for the first 5 frames
348+
for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
349+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
350+
const { scopeChain, functionName, this: obj } = callFrames[i]!;
351+
352+
const localScope = scopeChain.find(scope => scope.type === 'local');
353+
354+
// obj.className is undefined in ESM modules
355+
const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;
356+
357+
if (localScope?.object.objectId === undefined) {
358+
add(frames => {
359+
frames[i] = { function: fn };
360+
next(frames);
361+
});
362+
} else {
363+
const id = localScope.object.objectId;
364+
add(frames =>
365+
session.getLocalVariables(id, vars => {
366+
frames[i] = { function: fn, vars };
360367
next(frames);
361-
});
362-
} else {
363-
const id = localScope.object.objectId;
364-
add(frames =>
365-
session.getLocalVariables(id, vars => {
366-
frames[i] = { function: fn, vars };
367-
next(frames);
368-
}),
369-
);
370-
}
368+
}),
369+
);
371370
}
371+
}
372372

373-
next([]);
374-
};
375-
376-
const captureAll = options.captureAllExceptions !== false;
377-
378-
session.configureAndConnect(
379-
(ev, complete) =>
380-
handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
381-
captureAll,
373+
next([]);
374+
};
375+
376+
const captureAll = options.captureAllExceptions !== false;
377+
378+
session.configureAndConnect(
379+
(ev, complete) =>
380+
handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
381+
captureAll,
382+
);
383+
384+
if (captureAll) {
385+
const max = options.maxExceptionsPerSecond || 50;
386+
387+
rateLimiter = createRateLimiter(
388+
max,
389+
() => {
390+
debug.log('Local variables rate-limit lifted.');
391+
session.setPauseOnExceptions(true);
392+
},
393+
seconds => {
394+
debug.log(
395+
`Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
396+
);
397+
session.setPauseOnExceptions(false);
398+
},
382399
);
400+
}
383401

384-
if (captureAll) {
385-
const max = options.maxExceptionsPerSecond || 50;
386-
387-
rateLimiter = createRateLimiter(
388-
max,
389-
() => {
390-
debug.log('Local variables rate-limit lifted.');
391-
session.setPauseOnExceptions(true);
392-
},
393-
seconds => {
394-
debug.log(
395-
`Local variables rate-limit exceeded. Disabling capturing of caught exceptions for ${seconds} seconds.`,
396-
);
397-
session.setPauseOnExceptions(false);
398-
},
399-
);
400-
}
402+
shouldProcessEvent = true;
403+
} catch (error) {
404+
debug.log('The `LocalVariables` integration failed to start.', error);
405+
}
406+
}
401407

402-
shouldProcessEvent = true;
403-
} catch (error) {
404-
debug.log('The `LocalVariables` integration failed to start.', error);
405-
}
408+
return {
409+
name: INTEGRATION_NAME,
410+
setupOnce() {
411+
setupPromise = setup();
406412
},
407-
processEvent(event: Event): Event {
413+
async processEvent(event: Event): Promise<Event> {
414+
await setupPromise;
415+
408416
if (shouldProcessEvent) {
409417
return addLocalVariablesToEvent(event);
410418
}

packages/node-core/test/integrations/local-variables.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,13 @@ describe('LocalVariables', () => {
2626
const eventName = 'test-exclude-LocalVariables-out-of-app-frames';
2727
const event = getTestEvent(eventName);
2828
const integration = localVariablesSyncIntegration({}, mockSession);
29-
await integration.setupOnce?.();
29+
integration.setupOnce?.();
3030

3131
const hash = hashFrames(event.exception!.values![0]!.stacktrace!.frames);
3232
// @ts-expect-error test helper method
3333
integration._setCachedFrame(hash!, [{ function: eventName, vars: { foo: 'bar' } } as FrameVariables]);
3434

35-
const processedEvent = integration.processEvent?.(event, {}, {} as any) as Event;
35+
const processedEvent = (await integration.processEvent?.(event, {}, {} as any)) as Event;
3636

3737
expect(processedEvent.exception?.values?.[0]?.stacktrace?.frames?.[0]?.vars).toBeUndefined();
3838
});
@@ -41,13 +41,13 @@ describe('LocalVariables', () => {
4141
const eventName = 'test-include-LocalVariables-out-of-app-frames';
4242
const event = getTestEvent(eventName);
4343
const integration = localVariablesSyncIntegration({ includeOutOfAppFrames: true }, mockSession);
44-
await integration.setupOnce?.();
44+
integration.setupOnce?.();
4545

4646
const hash = hashFrames(event.exception!.values![0]!.stacktrace!.frames);
4747
// @ts-expect-error test helper method
4848
integration._setCachedFrame(hash!, [{ function: eventName, vars: { foo: 'bar' } } as FrameVariables]);
4949

50-
const processedEvent = integration.processEvent?.(event, {}, {} as any) as Event;
50+
const processedEvent = (await integration.processEvent?.(event, {}, {} as any)) as Event;
5151

5252
expect(processedEvent.exception?.values?.[0]?.stacktrace?.frames?.[0]?.vars).toEqual({ foo: 'bar' });
5353
});

0 commit comments

Comments
 (0)