diff --git a/src/agent/config.ts b/src/agent/config.ts index e72e7117..45ef4f36 100644 --- a/src/agent/config.ts +++ b/src/agent/config.ts @@ -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 { @@ -406,4 +414,5 @@ export const defaultConfig: ResolvedDebugAgentConfig = { forceNewAgent_: false, testMode_: false, + resetV8DebuggerThreshold: 30, }; diff --git a/src/agent/v8/inspector-debugapi.ts b/src/agent/v8/inspector-debugapi.ts index 60dd2910..b71815ee 100644 --- a/src/agent/v8/inspector-debugapi.ts +++ b/src/agent/v8/inspector-debugapi.ts @@ -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 @@ -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. @@ -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, @@ -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( @@ -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]; @@ -297,7 +323,7 @@ export class InspectorDebugApi implements debugapi.DebugApi { } disconnect(): void { - this.session.disconnect(); + this.v8.session.disconnect(); } numBreakpoints_(): number { @@ -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:/// @@ -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 { @@ -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 { @@ -608,7 +637,7 @@ export class InspectorDebugApi implements debugapi.DebugApi { breakpoint, this.config, this.scriptMapper, - this.v8Inspector + this.v8.inspector ); if ( breakpoint.location && @@ -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); + } + } } } diff --git a/test/test-debuglet.ts b/test/test-debuglet.ts index 82369828..6c923236 100644 --- a/test/test-debuglet.ts +++ b/test/test-debuglet.ts @@ -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}> = []; diff --git a/test/test-v8debugapi.ts b/test/test-v8debugapi.ts index 741948bc..a27b7b02 100644 --- a/test/test-v8debugapi.ts +++ b/test/test-v8debugapi.ts @@ -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'; @@ -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