Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

Commit

Permalink
fix: periodically reset v8 session to prevent memory leak (#957)
Browse files Browse the repository at this point in the history 
* fix: periodically reset v8 session to prevent memory leak

This patch makes the cloud debugger reset the v8 debugger session to
prevent the memory leak caused by v8 breakpoint hits whenenever the
number of breakpoint hits reach the threshold.

The threshold can be set through environment variable.

Fixes #811

* fix: lint issue, remove reading from env about reset threshold

* fix: run lint on all the files

* fix: update comments for default value

Co-authored-by: Benjamin E. Coe <bencoe@google.com>
  • Loading branch information
@Louis-Ye @bcoe
Louis-Ye and bcoe committed May 31, 2021
1 parent b312004 commit 7735425
Show file tree
Hide file tree
Showing 4 changed files with 176 additions and 21 deletions.
9 changes: 9 additions & 0 deletions src/agent/config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,14 @@ export interface ResolvedDebugAgentConfig extends GoogleAuthOptions {
* used to set a default api url
*/
apiUrl?: string;

/**
* Number of times of the V8 breakpoint hits events before resetting the
* breakpoints. This is to release the memory usage held by V8 engine for each
* breakpoint hit to prevent the memory leak. The default value is specified
* in defaultConfig.
*/
resetV8DebuggerThreshold: number;
}

export interface StackdriverConfig extends GoogleAuthOptions {
Expand Down Expand Up @@ -406,4 +414,5 @@ export const defaultConfig: ResolvedDebugAgentConfig = {

forceNewAgent_: false,
testMode_: false,
resetV8DebuggerThreshold: 30,
};
103 changes: 82 additions & 21 deletions src/agent/v8/inspector-debugapi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,21 @@ interface InspectorOptions {
useWellFormattedUrl: boolean;
}

/** Data related to the v8 inspector. */
interface V8Data {
session: inspector.Session;
// Options for behavior when interfacing with the Inspector API.
inspectorOptions: InspectorOptions;
inspector: V8Inspector;
// Store the v8 setBreakpoint parameters for each v8 breakpoint so that later
// the recorded parameters can be used to reset the breakpoints.
setBreakpointsParams: {
[
v8BreakpointId: string
]: inspector.Debugger.SetBreakpointByUrlParameterType;
};
}

/**
* In older versions of Node, the script source as seen by the Inspector
* backend is wrapped in `require('module').wrapper`, and in new versions
Expand Down Expand Up @@ -68,7 +83,6 @@ export class InspectorDebugApi implements debugapi.DebugApi {
fileStats: ScanStats;
breakpoints: {[id: string]: BreakpointData} = {};
sourcemapper: SourceMapper;
session: inspector.Session;
// TODO: listeners, scrpitmapper, location mapper and breakpointmapper can use
// Map in the future after resolving Map.prototype.get(key) returns V or
// undefined.
Expand All @@ -81,9 +95,9 @@ export class InspectorDebugApi implements debugapi.DebugApi {
// stackdriver breakpoint id.
breakpointMapper: {[id: string]: stackdriver.BreakpointId[]} = {};
numBreakpoints = 0;
// Options for behavior when interfacing with the Inspector API.
private inspectorOptions: InspectorOptions;
v8Inspector: V8Inspector;
numBreakpointHitsBeforeReset = 0;
v8: V8Data;

constructor(
logger: consoleLogLevel.Logger,
config: ResolvedDebugAgentConfig,
Expand All @@ -94,25 +108,36 @@ export class InspectorDebugApi implements debugapi.DebugApi {
this.config = config;
this.fileStats = jsFiles;
this.sourcemapper = sourcemapper;
this.session = new inspector.Session();
this.session.connect();
this.session.on('Debugger.scriptParsed', script => {
this.scriptMapper = {};
this.v8 = this.createV8Data();
}

/** Creates a new V8 Debugging session and the related data. */
private createV8Data(): V8Data {
const session = new inspector.Session();
session.connect();
session.on('Debugger.scriptParsed', script => {
this.scriptMapper[script.params.scriptId] = script.params;
});
this.session.post('Debugger.enable');
this.session.post('Debugger.setBreakpointsActive', {active: true});
this.session.on('Debugger.paused', message => {
session.post('Debugger.enable');
session.post('Debugger.setBreakpointsActive', {active: true});
session.on('Debugger.paused', message => {
try {
this.handleDebugPausedEvent(message.params);
} catch (error) {
this.logger.error(error);
}
});
this.inspectorOptions = {
// Well-Formatted URL is required in Node 10.11.1+.
useWellFormattedUrl: utils.satisfies(process.version, '>10.11.0'),

return {
session,
inspectorOptions: {
// Well-Formatted URL is required in Node 10.11.1+.
useWellFormattedUrl: utils.satisfies(process.version, '>10.11.0'),
},
inspector: new V8Inspector(session),
setBreakpointsParams: {},
};
this.v8Inspector = new V8Inspector(this.session);
}

set(
Expand Down Expand Up @@ -219,7 +244,8 @@ export class InspectorDebugApi implements debugapi.DebugApi {
if (!this.breakpointMapper[breakpointData.id]) {
// When breakpointmapper does not countain current v8/inspector breakpoint
// id, we should remove this breakpoint from v8.
result = this.v8Inspector.removeBreakpoint(breakpointData.id);
result = this.v8.inspector.removeBreakpoint(breakpointData.id);
delete this.v8.setBreakpointsParams[breakpointData.id];
}
delete this.breakpoints[breakpoint.id];
delete this.listeners[breakpoint.id];
Expand Down Expand Up @@ -297,7 +323,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
}

disconnect(): void {
this.session.disconnect();
this.v8.session.disconnect();
}

numBreakpoints_(): number {
Expand Down Expand Up @@ -487,7 +513,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
let v8BreakpointId; // v8/inspector breakpoint id
if (!this.locationMapper[locationStr]) {
// The first time when a breakpoint was set to this location.
const rawUrl = this.inspectorOptions.useWellFormattedUrl
const rawUrl = this.v8.inspectorOptions.useWellFormattedUrl
? `file://${matchingScript}`
: matchingScript;
// on windows on Node 11+, the url must start with file:///
Expand All @@ -496,17 +522,20 @@ export class InspectorDebugApi implements debugapi.DebugApi {
process.platform === 'win32' && utils.satisfies(process.version, '>=11')
? rawUrl.replace(/^file:\/\//, 'file:///').replace(/\\/g, '/')
: rawUrl;
const res = this.v8Inspector.setBreakpointByUrl({
const params = {
lineNumber: line - 1,
url,
columnNumber: column - 1,
condition: breakpoint.condition || undefined,
});
};
const res = this.v8.inspector.setBreakpointByUrl(params);
if (res.error || !res.response) {
// Error case.
return null;
}
v8BreakpointId = res.response.breakpointId;
this.v8.setBreakpointsParams[v8BreakpointId] = params;

this.locationMapper[locationStr] = [];
this.breakpointMapper[v8BreakpointId] = [];
} else {
Expand Down Expand Up @@ -593,7 +622,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
const evaluatedExpressions = breakpoint.expressions.map(exp => {
// returnByValue is set to true here so that the JSON string of the
// value will be returned to log.
const result = state.evaluate(exp, frame, that.v8Inspector, true);
const result = state.evaluate(exp, frame, that.v8.inspector, true);
if (result.error) {
return result.error;
} else {
Expand All @@ -608,7 +637,7 @@ export class InspectorDebugApi implements debugapi.DebugApi {
breakpoint,
this.config,
this.scriptMapper,
this.v8Inspector
this.v8.inspector
);
if (
breakpoint.location &&
Expand Down Expand Up @@ -642,5 +671,37 @@ export class InspectorDebugApi implements debugapi.DebugApi {
} catch (e) {
this.logger.warn('Internal V8 error on breakpoint event: ' + e);
}

this.tryResetV8Debugger();
}

/**
* Periodically resets breakpoints to prevent memory leaks in V8 (for holding
* contexts of previous breakpoint hits).
*/
private tryResetV8Debugger() {
this.numBreakpointHitsBeforeReset += 1;
if (
this.numBreakpointHitsBeforeReset < this.config.resetV8DebuggerThreshold
) {
return;
}
this.numBreakpointHitsBeforeReset = 0;

const storedParams = this.v8.setBreakpointsParams;

// Re-connect the session to clean the memory usage.
this.disconnect();
this.scriptMapper = {};
this.v8 = this.createV8Data();
this.v8.setBreakpointsParams = storedParams;

// Setting the v8 breakpoints again according to the stored parameters.
for (const params of Object.values(storedParams)) {
const res = this.v8.inspector.setBreakpointByUrl(params);
if (res.error || !res.response) {
this.logger.error('Error upon re-setting breakpoint: ' + res);
}
}
}
}
14 changes: 14 additions & 0 deletions test/test-debuglet.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,20 @@ describe('Debuglet', () => {
debuglet.start();
});

it('should have default resetV8DebuggerThreshold value', done => {
const debuglet = new Debuglet(new Debug({}, packageInfo), {});
assert.strictEqual(debuglet.config.resetV8DebuggerThreshold, 30);
done();
});

it('should overwrite resetV8DebuggerThreshold when available', done => {
const debuglet = new Debuglet(new Debug({}, packageInfo), {
resetV8DebuggerThreshold: 123,
});
assert.strictEqual(debuglet.config.resetV8DebuggerThreshold, 123);
done();
});

it('should not fail if files cannot be read', done => {
const MOCKED_DIRECTORY = process.cwd();
const errors: Array<{filename: string; error: string}> = [];
Expand Down
71 changes: 71 additions & 0 deletions test/test-v8debugapi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import * as extend from 'extend';
import * as debugapi from '../src/agent/v8/debugapi';
import {defaultConfig} from '../src/agent/config';
import {StatusMessage} from '../src/client/stackdriver/status-message';
import {InspectorDebugApi} from '../src/agent/v8/inspector-debugapi';
import * as scanner from '../src/agent/io/scanner';
import * as SourceMapper from '../src/agent/io/sourcemapper';
import * as path from 'path';
Expand Down Expand Up @@ -722,6 +723,76 @@ describe('v8debugapi', () => {
});
});

describe('InspectorDebugApi', () => {
let oldLPS: number;
let oldDS: number;

before(() => {
oldLPS = config.log.maxLogsPerSecond;
oldDS = config.log.logDelaySeconds;
config.log.maxLogsPerSecond = config.resetV8DebuggerThreshold * 3;
config.log.logDelaySeconds = 1;
});

after(() => {
config.log.maxLogsPerSecond = oldLPS;
config.log.logDelaySeconds = oldDS;
assert(stateIsClean(api));
});

it('should perform v8 breakpoints reset when meeting threshold', done => {
// The test is only eligible for the InspectorDebugApi test target.
if (!(api instanceof InspectorDebugApi)) {
done();
return;
}

const bp: stackdriver.Breakpoint = {
id: breakpointInFoo.id,
location: breakpointInFoo.location,
action: 'LOG',
logMessageFormat: 'cat',
} as {} as stackdriver.Breakpoint;
api.set(bp, err1 => {
let logpointEvaluatedTimes = 0;

assert.ifError(err1);
api.log(
bp,
() => {
logpointEvaluatedTimes += 1;
},
() => false
);

const inspectorDebugApi = api as InspectorDebugApi;
const v8BeforeReset = inspectorDebugApi.v8;

// The loop should trigger the breakpoints reset.
for (let i = 0; i < config.resetV8DebuggerThreshold; i++) {
code.foo(1);
}

// Expect the current v8 data is no longer the previous one.
assert.notStrictEqual(inspectorDebugApi.v8, v8BeforeReset);

// Make sure the logpoint is still triggered correctly after the second reset.
for (let i = 0; i < config.resetV8DebuggerThreshold + 1; i++) {
code.foo(1);
}
assert.strictEqual(
logpointEvaluatedTimes,
config.resetV8DebuggerThreshold * 2 + 1
);

api.clear(bp, err2 => {
assert.ifError(err2);
done();
});
});
});
});

describe('set and wait', () => {
it('should be possible to wait on a breakpoint', done => {
// clone a clean breakpointInFoo
Expand Down

0 comments on commit 7735425

Please sign in to comment.