Skip to content

fix(node): Convert debugging code to callbacks to fix memory leak in LocalVariables integration #7637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 29, 2023
Merged

fix(node): Convert debugging code to callbacks to fix memory leak in LocalVariables integration #7637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8df36e0
Use callbacks rather than async
timfish Mar 28, 2023
ab82a2e
Typos and small fixes
timfish Mar 29, 2023
eeb605b
Fixes
timfish Mar 29, 2023
623e048
Added more tests
timfish Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/node-integration-tests/suites/public-api/LocalVariables/local-variables-caught.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ function one(name) {
const obj = {
name,
num: 5,
functionsShouldNotBeIncluded: () => {},
functionsShouldNotBeIncluded2() {},
};

const ty = new Some();
Expand Down
230 changes: 155 additions & 75 deletions packages/node/src/integrations/localvariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,53 @@ import { LRUMap } from 'lru_map';

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

type Variables = Record<string, unknown>;
type OnPauseEvent = InspectorNotification<Debugger.PausedEventDataType>;
export interface DebugSession {
/** Configures and connects to the debug session */
configureAndConnect(
onPause: (message: InspectorNotification<Debugger.PausedEventDataType>) => void,
captureAll: boolean,
): void;
configureAndConnect(onPause: (message: OnPauseEvent, complete: () => void) => void, captureAll: boolean): void;
/** Gets local variables for an objectId */
getLocalVariables(objectId: string): Promise<Record<string, unknown>>;
getLocalVariables(objectId: string, callback: (vars: Variables) => void): void;
}

type Next<T> = (result: T) => void;
type Add<T> = (fn: Next<T>) => void;
type CallbackWrapper<T> = { add: Add<T>; next: Next<T> };

/** Creates a container for callbacks to be called sequentially */
export function createCallbackList<T>(complete: Next<T>): CallbackWrapper<T> {
// A collection of callbacks to be executed last to first
let callbacks: Next<T>[] = [];

let completedCalled = false;
function checkedComplete(result: T): void {
callbacks = [];
if (completedCalled) {
return;
}
completedCalled = true;
complete(result);
}

// complete should be called last
callbacks.push(checkedComplete);

function add(fn: Next<T>): void {
callbacks.push(fn);
}

function next(result: T): void {
const popped = callbacks.pop() || checkedComplete;

try {
popped(result);
} catch (_) {
// If there is an error, we still want to call the complete callback
checkedComplete(result);
}
}

return { add, next };
}

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

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

/** @inheritdoc */
public configureAndConnect(
onPause: (message: InspectorNotification<Debugger.PausedEventDataType>) => void,
captureAll: boolean,
): void {
public configureAndConnect(onPause: (event: OnPauseEvent, complete: () => void) => void, captureAll: boolean): void {
this._session.connect();
this._session.on('Debugger.paused', onPause);

this._session.on('Debugger.paused', event => {
onPause(event, () => {
// After the pause work is complete, resume execution or the exception context memory is leaked
this._session.post('Debugger.resume');
});
});

this._session.post('Debugger.enable');
// We only want to pause on uncaught exceptions
this._session.post('Debugger.setPauseOnExceptions', { state: captureAll ? 'all' : 'uncaught' });
}

/** @inheritdoc */
public async getLocalVariables(objectId: string): Promise<Record<string, unknown>> {
const props = await this._getProperties(objectId);
const unrolled: Record<string, unknown> = {};

for (const prop of props) {
if (prop?.value?.objectId && prop?.value.className === 'Array') {
unrolled[prop.name] = await this._unrollArray(prop.value.objectId);
} else if (prop?.value?.objectId && prop?.value?.className === 'Object') {
unrolled[prop.name] = await this._unrollObject(prop.value.objectId);
} else if (prop?.value?.value || prop?.value?.description) {
unrolled[prop.name] = prop.value.value || `<${prop.value.description}>`;
public getLocalVariables(objectId: string, complete: (vars: Variables) => void): void {
this._getProperties(objectId, props => {
const { add, next } = createCallbackList<Variables>(complete);

for (const prop of props) {
if (prop?.value?.objectId && prop?.value.className === 'Array') {
const id = prop.value.objectId;
add(vars => this._unrollArray(id, prop.name, vars, next));
} else if (prop?.value?.objectId && prop?.value?.className === 'Object') {
const id = prop.value.objectId;
add(vars => this._unrollObject(id, prop.name, vars, next));
} else if (prop?.value?.value || prop?.value?.description) {
add(vars => this._unrollOther(prop, vars, next));
}
}
}

return unrolled;
next({});
});
}

/**
* Gets all the PropertyDescriptors of an object
*/
private _getProperties(objectId: string): Promise<Runtime.PropertyDescriptor[]> {
return new Promise((resolve, reject) => {
this._session.post(
'Runtime.getProperties',
{
objectId,
ownProperties: true,
},
(err, params) => {
if (err) {
reject(err);
} else {
resolve(params.result);
}
},
);
});
private _getProperties(objectId: string, next: (result: Runtime.PropertyDescriptor[]) => void): void {
this._session.post(
'Runtime.getProperties',
{
objectId,
ownProperties: true,
},
(err, params) => {
if (err) {
next([]);
} else {
next(params.result);
}
},
);
}

/**
* Unrolls an array property
*/
private async _unrollArray(objectId: string): Promise<unknown> {
const props = await this._getProperties(objectId);
return props
.filter(v => v.name !== 'length' && !isNaN(parseInt(v.name, 10)))
.sort((a, b) => parseInt(a.name, 10) - parseInt(b.name, 10))
.map(v => v?.value?.value);
private _unrollArray(objectId: string, name: string, vars: Variables, next: (vars: Variables) => void): void {
this._getProperties(objectId, props => {
vars[name] = props
.filter(v => v.name !== 'length' && !isNaN(parseInt(v.name, 10)))
.sort((a, b) => parseInt(a.name, 10) - parseInt(b.name, 10))
.map(v => v?.value?.value);

next(vars);
});
}

/**
* Unrolls an object property
*/
private async _unrollObject(objectId: string): Promise<Record<string, unknown>> {
const props = await this._getProperties(objectId);
return props
.map<[string, unknown]>(v => [v.name, v?.value?.value])
.reduce((obj, [key, val]) => {
obj[key] = val;
return obj;
}, {} as Record<string, unknown>);
private _unrollObject(objectId: string, name: string, vars: Variables, next: (obj: Variables) => void): void {
this._getProperties(objectId, props => {
vars[name] = props
.map<[string, unknown]>(v => [v.name, v?.value?.value])
.reduce((obj, [key, val]) => {
obj[key] = val;
return obj;
}, {} as Variables);

next(vars);
});
}

/**
* Unrolls other properties
*/
private _unrollOther(prop: Runtime.PropertyDescriptor, vars: Variables, next: (vars: Variables) => void): void {
if (prop?.value?.value) {
vars[prop.name] = prop.value.value;
} else if (prop?.value?.description && prop?.value?.type !== 'function') {
vars[prop.name] = `<${prop.value.description}>`;
}

next(vars);
}
}

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

export interface FrameVariables {
function: string;
vars?: Record<string, unknown>;
vars?: Variables;
}

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

public readonly name: string = LocalVariables.id;

private readonly _cachedFrames: LRUMap<string, Promise<FrameVariables[]>> = new LRUMap(20);
private readonly _cachedFrames: LRUMap<string, FrameVariables[]> = new LRUMap(20);

public constructor(
private readonly _options: Options = {},
Expand All @@ -221,7 +283,8 @@ export class LocalVariables implements Integration {
): void {
if (this._session && clientOptions?.includeLocalVariables) {
this._session.configureAndConnect(
ev => this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>),
(ev, complete) =>
this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
!!this._options.captureAllExceptions,
);

Expand All @@ -232,47 +295,64 @@ export class LocalVariables implements Integration {
/**
* Handle the pause event
*/
private async _handlePaused(
private _handlePaused(
stackParser: StackParser,
{ params: { reason, data, callFrames } }: InspectorNotification<PausedExceptionEvent>,
): Promise<void> {
complete: () => void,
): void {
if (reason !== 'exception' && reason !== 'promiseRejection') {
complete();
return;
}

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

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

const framePromises = callFrames.map(async ({ scopeChain, functionName, this: obj }) => {
const { add, next } = createCallbackList<FrameVariables[]>(frames => {
this._cachedFrames.set(exceptionHash, frames);
complete();
});

// Because we're queuing up and making all these calls synchronously, we can potentially overflow the stack
// For this reason we only attempt to get local variables for the first 5 frames
for (let i = 0; i < Math.min(callFrames.length, 5); i++) {
const { scopeChain, functionName, this: obj } = callFrames[i];

const localScope = scopeChain.find(scope => scope.type === 'local');

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

if (localScope?.object.objectId === undefined) {
return { function: fn };
add(frames => {
frames[i] = { function: fn };
next(frames);
});
} else {
const id = localScope.object.objectId;
add(frames =>
this._session?.getLocalVariables(id, vars => {
frames[i] = { function: fn, vars };
next(frames);
}),
);
}
}

const vars = await this._session?.getLocalVariables(localScope.object.objectId);

return { function: fn, vars };
});

// We add the un-awaited promise to the cache rather than await here otherwise the event processor
// can be called before we're finished getting all the vars
this._cachedFrames.set(exceptionHash, Promise.all(framePromises));
next([]);
}

/**
* Adds local variables event stack frames.
*/
private async _addLocalVariables(event: Event): Promise<Event> {
private _addLocalVariables(event: Event): Event {
for (const exception of event?.exception?.values || []) {
await this._addLocalVariablesToException(exception);
this._addLocalVariablesToException(exception);
}

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

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

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

if (cachedFrames === undefined) {
return;
Expand Down
Loading