From 031a7ad5c830beee318ae36a9e56b6588bc929d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20Ye=20=E5=8F=B6=E7=BB=BF=E5=AE=87?= Date: Thu, 17 Jun 2021 09:28:46 -0400 Subject: [PATCH] fix: attach to v8 debugger session only when having active breakpoints (#975) * fix: attach to v8 debugger session only when having active breakpoints * This PR also refactos the inspector-debugapi class so that the V8 related code is moved to v8inspector class. * Update comments to be more accurate Co-authored-by: Justin Beckwith Co-authored-by: James McTavish --- src/agent/config.ts | 8 +- src/agent/v8/inspector-debugapi.ts | 130 +++++++--------------------- src/agent/v8/v8inspector.ts | 134 ++++++++++++++++++++++++++--- test/test-v8debugapi.ts | 46 +++++++++- 4 files changed, 198 insertions(+), 120 deletions(-) diff --git a/src/agent/config.ts b/src/agent/config.ts index 45ef4f36..6b325a14 100644 --- a/src/agent/config.ts +++ b/src/agent/config.ts @@ -359,10 +359,10 @@ export interface ResolvedDebugAgentConfig extends GoogleAuthOptions { 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. + * Number of times the V8 pauses (could be breakpoint hits) before the + * debugging session is reset. 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; } diff --git a/src/agent/v8/inspector-debugapi.ts b/src/agent/v8/inspector-debugapi.ts index 8e1b4807..c47522fb 100644 --- a/src/agent/v8/inspector-debugapi.ts +++ b/src/agent/v8/inspector-debugapi.ts @@ -34,32 +34,6 @@ import * as utils from '../util/utils'; import * as debugapi from './debugapi'; import {V8Inspector} from './v8inspector'; -/** - * An interface that describes options that set behavior when interacting with - * the V8 Inspector API. - */ -interface InspectorOptions { - /** - * Whether to add a 'file://' prefix to a URL when setting breakpoints. - */ - 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 @@ -99,8 +73,9 @@ export class InspectorDebugApi implements debugapi.DebugApi { // stackdriver breakpoint id. breakpointMapper: {[id: string]: stackdriver.BreakpointId[]} = {}; numBreakpoints = 0; - numBreakpointHitsBeforeReset = 0; - v8: V8Data; + + // The wrapper class that interacts with the V8 debugger. + inspector: V8Inspector; constructor( logger: consoleLogLevel.Logger, @@ -112,36 +87,28 @@ export class InspectorDebugApi implements debugapi.DebugApi { this.config = config; this.fileStats = jsFiles; this.sourcemapper = sourcemapper; - 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; - }); - 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.inspector = new V8Inspector( + /* logger=*/ logger, + /*useWellFormattedUrl=*/ utils.satisfies(process.version, '>10.11.0'), + /*resetV8DebuggerThreshold=*/ this.config.resetV8DebuggerThreshold, + /*onScriptParsed=*/ + scriptParams => { + this.scriptMapper[scriptParams.scriptId] = scriptParams; + }, + /*onPaused=*/ + messageParams => { + try { + this.handleDebugPausedEvent(messageParams); + } catch (error) { + this.logger.error(error); + } } - }); + ); + } - 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: {}, - }; + /** Disconnects and marks the current V8 data as not connected. */ + disconnect(): void { + this.inspector.detach(); } set( @@ -268,8 +235,7 @@ 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.v8.inspector.removeBreakpoint(breakpointData.id); - delete this.v8.setBreakpointsParams[breakpointData.id]; + result = this.inspector.removeBreakpoint(breakpointData.id); } delete this.breakpoints[breakpoint.id]; delete this.listeners[breakpoint.id]; @@ -346,10 +312,6 @@ export class InspectorDebugApi implements debugapi.DebugApi { this.listeners[breakpoint.id] = {enabled: true, listener}; } - disconnect(): void { - this.v8.session.disconnect(); - } - numBreakpoints_(): number { // Tracks the number of stackdriver breakpoints. return Object.keys(this.breakpoints).length; @@ -537,7 +499,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.v8.inspectorOptions.useWellFormattedUrl + const rawUrl = this.inspector.shouldUseWellFormattedUrl() ? `file://${matchingScript}` : matchingScript; // on windows on Node 11+, the url must start with file:/// @@ -546,19 +508,17 @@ export class InspectorDebugApi implements debugapi.DebugApi { process.platform === 'win32' && utils.satisfies(process.version, '>=11') ? rawUrl.replace(/^file:\/\//, 'file:///').replace(/\\/g, '/') : rawUrl; - const params = { + const res = this.inspector.setBreakpointByUrl({ 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] = []; @@ -646,7 +606,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.v8.inspector, true); + const result = state.evaluate(exp, frame, that.inspector, true); if (result.error) { return result.error; } else { @@ -661,7 +621,7 @@ export class InspectorDebugApi implements debugapi.DebugApi { breakpoint, this.config, this.scriptMapper, - this.v8.inspector + this.inspector ); if ( breakpoint.location && @@ -695,37 +655,5 @@ 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/src/agent/v8/v8inspector.ts b/src/agent/v8/v8inspector.ts index bc17ec69..0e6e5afb 100644 --- a/src/agent/v8/v8inspector.ts +++ b/src/agent/v8/v8inspector.ts @@ -15,26 +15,61 @@ // eslint-disable-next-line node/no-unsupported-features/node-builtins import * as inspector from 'inspector'; +import consoleLogLevel = require('console-log-level'); + export class V8Inspector { - private session: inspector.Session; - constructor(session: inspector.Session) { - this.session = session; + // The V8 debugger session. + session: inspector.Session | null = null; + + // Store of the v8 setBreakpoint parameters for each v8 breakpoint so that + // later the recorded parameters can be used to reset the breakpoints. + storeSetBreakpointParams: { + [ + v8BreakpointId: string + ]: inspector.Debugger.SetBreakpointByUrlParameterType; + } = {}; + + // Number of paused events before the next reset. + numPausedBeforeReset = 0; + + constructor( + readonly logger: consoleLogLevel.Logger, + readonly useWellFormattedUrl: boolean, + readonly resetV8DebuggerThreshold: number, + readonly onScriptParsed: ( + script: inspector.Debugger.ScriptParsedEventDataType + ) => void, + readonly onPaused: (params: inspector.Debugger.PausedEventDataType) => void + ) {} + + /** + * Whether to add a 'file://' prefix to a URL when setting breakpoints. + */ + shouldUseWellFormattedUrl() { + return this.useWellFormattedUrl; } + setBreakpointByUrl( - options: inspector.Debugger.SetBreakpointByUrlParameterType + params: inspector.Debugger.SetBreakpointByUrlParameterType ) { + this.attach(); + const result: { error?: Error; response?: inspector.Debugger.SetBreakpointByUrlReturnType; } = {}; - this.session.post( + this.session!.post( 'Debugger.setBreakpointByUrl', - options, + params, ( error: Error | null, response: inspector.Debugger.SetBreakpointByUrlReturnType ) => { - if (error) result.error = error; + if (error) { + result.error = error; + } else { + this.storeSetBreakpointParams[response.breakpointId] = params; + } result.response = response; } ); @@ -42,12 +77,23 @@ export class V8Inspector { } removeBreakpoint(breakpointId: string) { + this.attach(); + const result: {error?: Error} = {}; - this.session.post( + this.session!.post( 'Debugger.removeBreakpoint', {breakpointId}, (error: Error | null) => { - if (error) result.error = error; + if (error) { + result.error = error; + } else { + delete this.storeSetBreakpointParams[breakpointId]; + } + + // If there is no active V8 breakpoints, then detach the session. + if (Object.keys(this.storeSetBreakpointParams).length === 0) { + this.detach(); + } } ); return result; @@ -56,11 +102,13 @@ export class V8Inspector { evaluateOnCallFrame( options: inspector.Debugger.EvaluateOnCallFrameParameterType ) { + this.attach(); + const result: { error?: Error; response?: inspector.Debugger.EvaluateOnCallFrameReturnType; } = {}; - this.session.post( + this.session!.post( 'Debugger.evaluateOnCallFrame', options, ( @@ -79,7 +127,9 @@ export class V8Inspector { error?: Error; response?: inspector.Runtime.GetPropertiesReturnType; } = {}; - this.session.post( + this.attach(); + + this.session!.post( 'Runtime.getProperties', options, ( @@ -92,4 +142,66 @@ export class V8Inspector { ); return result; } + + /** Attaches to the V8 debugger. */ + private attach() { + if (this.session) { + return; + } + + const session = new inspector.Session(); + session.connect(); + session.on('Debugger.scriptParsed', script => { + this.onScriptParsed(script.params); + }); + session.post('Debugger.enable'); + session.post('Debugger.setBreakpointsActive', {active: true}); + session.on('Debugger.paused', message => { + this.onPaused(message.params); + this.resetV8DebuggerIfThresholdMet(); + }); + + this.session = session; + } + + /** + * Detaches from the V8 debugger. This will purge all the existing V8 + * breakpoints from the V8 debugger. + */ + detach() { + if (!this.session) { + return; + } + + this.session!.disconnect(); + this.session = null; + this.storeSetBreakpointParams = {}; + this.numPausedBeforeReset = 0; + } + + /** + * Resets the debugging session when the number of paused events meets the + * threshold. This is primarily for cleaning the memory usage hold by V8 + * debugger when hitting the V8 breakpoints too many times. + */ + private resetV8DebuggerIfThresholdMet() { + this.numPausedBeforeReset += 1; + if (this.numPausedBeforeReset < this.resetV8DebuggerThreshold) { + return; + } + this.numPausedBeforeReset = 0; + + const previouslyStoredParams = this.storeSetBreakpointParams; + + this.detach(); + this.attach(); + + // Setting the v8 breakpoints again according to the stored parameters. + for (const params of Object.values(previouslyStoredParams)) { + const res = this.setBreakpointByUrl(params); + if (res.error || !res.response) { + this.logger.error('Error upon re-setting breakpoint: ' + res); + } + } + } } diff --git a/test/test-v8debugapi.ts b/test/test-v8debugapi.ts index db771b42..54a8a35a 100644 --- a/test/test-v8debugapi.ts +++ b/test/test-v8debugapi.ts @@ -766,7 +766,41 @@ describe('v8debugapi', () => { assert(stateIsClean(api)); }); - it('should perform v8 breakpoints reset when meeting threshold', done => { + it('should only attach to v8 when having active breakpoints', done => { + // The test is only eligible for the InspectorDebugApi test target. + if (!(api instanceof InspectorDebugApi)) { + done(); + return; + } + + const inspectorDebugApi = api as InspectorDebugApi; + assert.strictEqual(inspectorDebugApi.inspector.session, null); + + const bp: stackdriver.Breakpoint = { + id: breakpointInFoo.id, + location: breakpointInFoo.location, + action: 'LOG', + logMessageFormat: 'cat', + } as {} as stackdriver.Breakpoint; + api.set(bp, err1 => { + assert.ifError(err1); + api.log( + bp, + () => {}, + () => false + ); + + assert.notStrictEqual(inspectorDebugApi.inspector.session, null); + + api.clear(bp, err2 => { + assert.ifError(err2); + assert.strictEqual(inspectorDebugApi.inspector.session, null); + done(); + }); + }); + }); + + it('should reset v8 debugging session when meeting threshold', done => { // The test is only eligible for the InspectorDebugApi test target. if (!(api instanceof InspectorDebugApi)) { done(); @@ -792,7 +826,7 @@ describe('v8debugapi', () => { ); const inspectorDebugApi = api as InspectorDebugApi; - const v8BeforeReset = inspectorDebugApi.v8; + const v8SessionBeforeReset = inspectorDebugApi.inspector.session; // The loop should trigger the breakpoints reset. for (let i = 0; i < config.resetV8DebuggerThreshold; i++) { @@ -800,9 +834,13 @@ describe('v8debugapi', () => { } // Expect the current v8 data is no longer the previous one. - assert.notStrictEqual(inspectorDebugApi.v8, v8BeforeReset); + assert.notStrictEqual( + inspectorDebugApi.inspector.session, + v8SessionBeforeReset + ); - // Make sure the logpoint is still triggered correctly after the second reset. + // Make sure the logpoint is still triggered correctly after the second + // reset. for (let i = 0; i < config.resetV8DebuggerThreshold + 1; i++) { code.foo(1); }