8000 fix(node): Convert debugging code to callbacks to fix memory leak in … · ileonovdima/sentry-javascript@8a25d00 · GitHub
[go: up one dir, main page]
Skip to content

Commit 8a25d00

Browse files
timfishtimfish
authored
fix(node): Convert debugging code to callbacks to fix memory leak in LocalVariables integration (getsentry#7637)
1 parent 0643828 commit 8a25d00

File tree

3 files changed

+318
-94
lines changed

3 files changed

+318
-94
lines changed

‎packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-caught.mjs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ function one(name) {
2222
const obj = {
2323
name,
2424
num: 5,
25+
functionsShouldNotBeIncluded: () => {},
26+
functionsShouldNotBeIncluded2() {},
2527
};
2628

2729
const ty = new Some();

‎packages/node/src/integrations/localvariables.ts

Lines changed: 155 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,53 @@ import { LRUMap } from 'lru_map';
44

55
import type { NodeClientOptions } from '../types';
66

7+
type Variables = Record<string, unknown>;
8+
type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
79
export interface DebugSession {
810
/** Configures and connects to the debug session */
9-
configureAndConnect(
10-
onPause: (message: InspectorNotification<Debugger.PausedEventDataType>) => void,
11-
captureAll: boolean,
12-
): void;
11+
configureAndConnect(onPause: (message: OnPauseEvent, complete: () => void) => void, captureAll: boolean): void;
1312
/** Gets local variables for an objectId */
14-
getLocalVariables(objectId: string): Promise<Record<string, unknown>>;
13+
getLocalVariables(objectId: string, callback: (vars: Variables) => void): void;
14+
}
15+
16+
type Next<T> = (result: T) => void;
17+
type Add<T> = (fn: Next<T>) => void;
18+
type CallbackWrapper<T> = { add: Add<T>; next: Next<T> };
19+
20+
/** Creates a container for callbacks to be called sequentially */
21+
export function createCallbackList<T>(complete: Next<T>): CallbackWrapper<T> {
22+
// A collection of callbacks to be executed last to first
23+
let callbacks: Next<T>[] = [];
24+
25+
let completedCalled = false;
26+
function checkedComplete(result: T): void {
27+
callbacks = [];
28+
if (completedCalled) {
29+
return;
30+
}
31+
completedCalled = true;
32+
complete(result);
33+
}
34+
35+
// complete should be called last
36+
callbacks.push(checkedComplete);
37+
38+
function add(fn: Next<T>): void {
39+
callbacks.push(fn);
40+
}
41+
42+
function next(result: T): void {
43+
const popped = callbacks.pop() || checkedComplete;
44+
45+
try {
46+
popped(result);
47+
} catch (_) {
48+
// If there is an error, we still want to call the complete callback
49+
checkedComplete(result);
50+
}
51+
}
52+
53+
return { add, next };
1554
}
1655

1756
/**
@@ -41,86 +80,109 @@ class AsyncSession implements DebugSession {
4180
were reported any more. We probably missed a place where we need to await the promise, too.
4281
*/
4382

44-
// Node can be build without inspector support so this can throw
83+
// Node can be built without inspector support so this can throw
4584
// eslint-disable-next-line @typescript-eslint/no-var-requires
4685
const { Session } = require('inspector');
4786
this._session = new Session();
4887
}
4988

5089
/** @inheritdoc */
51-
public configureAndConnect(
52-
onPause: (message: InspectorNotification<Debugger.PausedEventDataType>) => void,
53-
captureAll: boolean,
54-
): void {
90+
public configureAndConnect(onPause: (event: OnPauseEvent, complete: () => void) => void, captureAll: boolean): void {
5591
this._session.connect();
56-
this._session.on('Debugger.paused', onPause);
92+
93+
this._session.on('Debugger.paused', event => {
94+
onPause(event, () => {
95+
// After the pause work is complete, resume execution or the exception context memory is leaked
96+
this._session.post('Debugger.resume');
97+
});
98+
});
99+
57100
this._session.post('Debugger.enable');
58-
// We only want to pause on uncaught exceptions
59101
this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' });
60102
}
61103

62104
/** @inheritdoc */
63-
public async getLocalVariables(objectId: string): Promise<Record<string, unknown>> {
64-
const props = await this._getProperties(objectId);
65-
const unrolled: Record<string, unknown> = {};
66-
67-
for (const prop of props) {
68-
if (prop?.value?.objectId && prop?.value.className === 'Array') {
69-
unrolled[prop.name] = await this._unrollArray(prop.value.objectId);
70-
} else if (prop?.value?.objectId && prop?.value?.className === 'Object') {
71-
unrolled[prop.name] = await this._unrollObject(prop.value.objectId);
72-
} else if (prop?.value?.value || prop?.value?.description) {
73-
unrolled[prop.name] = prop.value.value || `<${prop.value.description}>`;
105+
public getLocalVariables(objectId: string, complete: (vars: Variables) => void): void {
106+
this._getProperties(objectId, props => {
107+
const { add, next } = createCallbackList<Variables>(complete);
108+
109+
for (const prop of props) {
110+
if (prop?.value?.objectId && prop?.value.className === 'Array') {
111+
const id = prop.value.objectId;
112+
add(vars => this._unrollArray(id, prop.name, vars, next));
113+
} else if (prop?.value?.objectId && prop?.value?.className === 'Object') {
114+
const id = prop.value.objectId;
115+
add(vars => this._unrollObject(id, prop.name, vars, next));
116+
} else if (prop?.value?.value || prop?.value?.description) {
117+
add(vars => this._unrollOther(prop, vars, next));
118+
}
74119
}
75-
}
76120

77-
return unrolled;
121+
next({});
122+
});
78123
}
79124

80125
/**
81126
* Gets all the PropertyDescriptors of an object
82127
*/
83-
private _getProperties(objectId: string): Promise<Runtime.PropertyDescriptor[]> {
84-
return new Promise((resolve, reject) => {
85-
this._session.post(
86-
'Runtime.getProperties',
87-
{
88-
objectId,
89-
ownProperties: true,
90-
},
91-
(err, params) => {
92-
if (err) {
93-
reject(err);
94-
} else {
95-
resolve(params.result);
96-
}
97-
},
98-
);
99-
});
128+
private _getProperties(objectId: string, next: (result: Runtime.PropertyDescriptor[]) => void): void {
129+
this._session.post(
130+
'Runtime.getProperties',
131+
{
132+
objectId,
133+
ownProperties: true,
134+
},
135+
(err, params) => {
136+
if (err) {
137+
next([]);
138+
} else {
139+
next(params.result);
140+
}
141+
},
142+
);
100143
}
101144

102145
/**
103146
* Unrolls an array property
104147
*/
105-
private async _unrollArray(objectId: string): Promise<unknown> {
106-
const props = await this._getProperties(objectId);
107-
return props
108-
.filter(v => v.name !== 'length' && !isNaN(parseInt(v.name, 10)))
109-
.sort((a, b) => parseInt(a.name, 10) - parseInt(b.name, 10))
110-
.map(v => v?.value?.value);
148+
private _unrollArray(objectId: string, name: string, vars: Variables, next: (vars: Variables) => void): void {
149+
this._getProperties(objectId, props => {
150+
vars[name] = props
151+
.filter(v => v.name !== 'length' && !isNaN(parseInt(v.name, 10)))
152+
.sort((a, b) => parseInt(a.name, 10) - parseInt(b.name, 10))
153+
.map(v => v?.value?.value);
154+
155+
next(vars);
156+
});
111157
}
112158

113159
/**
114160
* Unrolls an object property
115161
*/
116-
private async _unrollObject(objectId: string): Promise<Record<string, unknown>> {
117-
const props = await this._getProperties(objectId);
118-
return props
119-
.map<[string, unknown]>(v => [v.name, v?.value?.value])
120-
.reduce((obj, [key, val]) => {
121-
obj[key] = val;
122-
return obj;
123-
}, {} as Record<string, unknown>);
162+
private _unrollObject(objectId: string, name: string, vars: Variables, next: (obj: Variables) => void): void {
163+
this._getProperties(objectId, props => {
164+
vars[name] = props
165+
.map<[string, unknown]>(v => [v.name, v?.value?.value])
166+
.reduce((obj, [key, val]) => {
167+
obj[key] = val;
168+
return obj;
169+
}, {} as Variables);
170+
171+
next(vars);
172+
});
173+
}
174+
175+
/**
176+
* Unrolls other properties
177+
*/
178+
private _unrollOther(prop: Runtime.PropertyDescriptor, vars: Variables, next: (vars: Va 741A riables) => void): void {
179+
if (prop?.value?.value) {
180+
vars[prop.name] = prop.value.value;
181+
} else if (prop?.value?.description && prop?.value?.type !== 'function') {
182+
vars[prop.name] = `<${prop.value.description}>`;
183+
}
184+
185+
next(vars);
124186
}
125187
}
126188

@@ -178,7 +240,7 @@ function hashFromStack(stackParser: StackParser, stack: string | undefined): str
178240

179241
export interface FrameVariables {
180242
function: string;
181-
vars?: Record<string, unknown>;
243+
vars?: Variables;
182244
}
183245

184246
/** There are no options yet. This allows them to be added later without breaking changes */
@@ -200,7 +262,7 @@ export class LocalVariables implements Integration {
200262

201263
public readonly name: string = LocalVariables.id;
202264

203-
private readonly _cachedFrames: LRUMap<string, Promise<FrameVariables[]>> = new LRUMap(20);
265+
private readonly _cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);
204266

205267
public constructor(
206268
private readonly _options: Options = {},
@@ -221,7 +283,8 @@ export class LocalVariables implements Integration {
221283
): void {
222284
if (this._session && clientOptions?.includeLocalVariables) {
223285
this._session.configureAndConnect(
224-
ev => this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>),
286+
(ev, complete) =>
287+
this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
225288
!!this._options.captureAllExceptions,
226289
);
227290

@@ -232,47 +295,64 @@ export class LocalVariables implements Integration {
232295
/**
233296
* Handle the pause event
234297
*/
235-
private async _handlePaused(
298+
private _handlePaused(
236299
stackParser: StackParser,
237300
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
238-
): Promise<void> {
301+
complete: () => void,
302+
): void {
239303
if (reason !== 'exception' && reason !== 'promiseRejection') {
304+
complete();
240305
return;
241306
}
242307

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

246311
if (exceptionHash == undefined) {
312+
complete();
247313
return;
248314
}
249315

250-
const framePromises = callFrames.map(async ({ scopeChain, functionName, this: obj }) => {
316+
const { add, next } = createCallbackList<FrameVariables[]>(frames => {
317+
this._cachedFrames.set(exceptionHash, frames);
318+
complete();
319+
});
320+
321+
// Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
322+
// For this reason we only attempt to get local variables for the first 5 frames
323+
for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
324+
const { scopeChain, functionName, this: obj } = callFrames[i];
325+
251326
const localScope = scopeChain.find(scope => scope.type === 'local');
252327

253328
// obj.className is undefined in ESM modules
254329
const fn = obj.className === 'global' || !obj.className ? functionName : `${obj.className}.${functionName}`;
255330

256331
if (localScope?.object.objectId === undefined) {
257-
return { function: fn };
332+
add(frames => {
333+
frames[i] = { function: fn };
334+
next(frames);
335+
});
336+
} else {
337+
const id = localScope.object.objectId;
338+
add(frames =>
339+
this._session?.getLocalVariables(id, vars => {
340+
frames[i] = { function: fn, vars };
341+
next(frames);
342+
}),
343+
);
258344
}
345+
}
259346

260-
const vars = await this._session?.getLocalVariables(localScope.object.objectId);
261-
262-
return { function: fn, vars };
263-
});
264-
265-
// We add the un-awaited promise to the cache rather than await here otherwise the event processor
266-
// can be called before we're finished getting all the vars
267-
this._cachedFrames.set(exceptionHash, Promise.all(framePromises));
347+
next([]);
268348
}
269349

270350
/**
271351
* Adds local variables event stack frames.
272352
*/
273-
private async _addLocalVariables(event: Event): Promise<Event> {
353+
private _addLocalVariables(event: Event): Event {
274354
for (const exception of event?.exception?.values || []) {
275-
await this._addLocalVariablesToException(exception);
355+
this._addLocalVariablesToException(exception);
276356
}
277357

278358
return event;
@@ -281,7 +361,7 @@ export class LocalVariables implements Integration {
281361
/**
282362
* Adds local variables to the exception stack frames.
283363
*/
284-
private async _addLocalVariablesToException(exception: Exception): Promise<void> {
364+
private _addLocalVariablesToException(exception: Exception): void {
285365
const hash = hashFrames(exception?.stacktrace?.frames);
286366

287367
if (hash === undefined) {
@@ -290,7 +370,7 @@ export class LocalVariables implements Integration {
290370

291371
// Check if we have local variables for an exception that matches the hash
292372
// delete is identical to get but also removes the entry from the cache
293-
const cachedFrames = await this._cachedFrames.delete(hash);
373+
const cachedFrames = this._cachedFrames.delete(hash);
294374

295375
if (cachedFrames === undefined) {
296376
return;

0 commit comments

Comments
 (0)
0