fix: periodically reset v8 session to prevent memory leak (#957)

* 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 <[email protected]>

Author: Louis-Ye

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,
 };

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);
+      }
+    }
   }
 }

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}> = [];

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