fix(backend): ndd agent cleanup (GoogleChromeLabs#74)

- node does not get 100% CPU anymore when waiting for a connection, - 'Stay attached' checkbox works for profiling, - simplified ndd agent. Fixes GoogleChromeLabs#73
nirnatan · Jul 28, 2018 · 79b4067 · 79b4067
1 parent 5462a41
commit 79b4067
Show file tree

Hide file tree

Showing 10 changed files with 147 additions and 157 deletions.
diff --git a/.npmignore b/.npmignore
@@ -1,6 +1,7 @@
 # exclude all tests
 test/
 test/fs/
+utils/testrunner
 
 # repeats from .gitignore
 /.local-frontend/

diff --git a/front_end/ndb/NdbMain.js b/front_end/ndb/NdbMain.js
@@ -286,7 +286,7 @@ Ndb.NodeProcessManager = class extends Common.Object {
   static async _create() {
     const service = await Ndb.serviceManager.create('ndd_service');
     const instance = new Ndb.NodeProcessManager(SDK.targetManager, service);
-    await service.call('start');
+    instance._nddStore = await service.call('start');
     Ndb.NodeProcessManager._instanceReady(instance);
     delete Ndb.NodeProcessManager._instanceReady;
   }
@@ -329,7 +329,7 @@ Ndb.NodeProcessManager = class extends Common.Object {
   }
 
   nddStore() {
-    return this._nddService.call('nddStore');
+    return this._nddStore;
   }
 
   _onNotification({data: {name, params}}) {
@@ -608,7 +608,7 @@ SDK.Target.prototype.decorateLabel = function(label) {
 };
 
 // Front-end does not respect modern toggle semantics, patch it.
-var originalToggle = DOMTokenList.prototype.toggle;
+const originalToggle = DOMTokenList.prototype.toggle;
 DOMTokenList.prototype.toggle = function(token, force) {
   if (arguments.length === 1)
     force = !this.contains(token);

diff --git a/front_end/ndb_ui/NodeProcesses.js b/front_end/ndb_ui/NodeProcesses.js
@@ -39,8 +39,8 @@ Ndb.NodeProcesses = class extends UI.VBox {
     this._pauseAtStartCheckbox.element.id = 'pause-at-start-checkbox';
     toolbar.appendToolbarItem(this._pauseAtStartCheckbox);
     this._waitAtEndCheckbox = new UI.ToolbarSettingCheckbox(
-        Common.moduleSetting('waitAtEnd'), Common.UIString('Wait for manual disconnect'),
-        Common.UIString('Wait at end'));
+        Common.moduleSetting('waitAtEnd'), Common.UIString('Stay attached when process is finished.'),
+        Common.UIString('Stay attached'));
     toolbar.appendToolbarItem(this._waitAtEndCheckbox);
 
     this._emptyElement = this.contentElement.createChild('div', 'gray-info-message');

diff --git a/lib/launcher.js b/lib/launcher.js
@@ -92,7 +92,8 @@ async function launch(options) {
   function cleanupAndExit() {
     services.dispose();
     fileSystemBackend.dispose();
-    process.exit(0);
+    if (!options.doNotProcessExit)
+      process.exit(0);
   }
 
   const overridesFolder = options.debugFrontend

diff --git a/node_debug_demon/client.js b/node_debug_demon/client.js
@@ -4,6 +4,7 @@
  * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
  */
 
+const chokidar = require('chokidar');
 const EventEmitter = require('events');
 const fs = require('fs');
 const path = require('path');
@@ -17,115 +18,62 @@ class Client extends EventEmitter {
     super();
     this._nddStore = nddStore;
     this._nddStoreWatcher = null;
-    this._watchEventListener = this._onWatchEvent.bind(this);
-
     this._instances = new Map();
-    this._updateScheduled = new Set();
   }
 
   async start() {
-    this._nddStoreWatcher = fs.watch(this._nddStore);
-    this._nddStoreWatcher.on('change', this._watchEventListener);
-    await this.cleanup();
-    await this._fetchExisting();
+    this._nddStoreWatcher = chokidar.watch([this._nddStore], {
+      awaitWriteFinish: {
+        stabilityThreshold: 500,
+        pollInterval: 100
+      },
+      cwd: this._nddStore,
+      depth: 0,
+      ignorePermissionErrors: true
+    });
+    this._nddStoreWatcher.on('add', path => {
+      const match = path.match(/([0-9]+)-ready/);
+      if (!match)
+        return;
+      const id = match[1];
+      let instance = this._instances.get(id);
+      if (instance)
+        return;
+      instance = new NodeProcess(id, this._nddStore);
+      this._instances.set(id, instance);
+      this.emit('added', instance);
+    });
+    this._nddStoreWatcher.on('unlink', path => {
+      const match = path.match(/([0-9]+)-ready/);
+      if (!match)
+        return;
+      const id = match[1];
+      const instance = this._instances.get(id);
+      if (!instance)
+        return;
+      this._instances.delete(id);
+      this.emit('finished', instance);
+    });
   }
 
   async stop() {
     const fileNames = await util.promisify(fs.readdir)(this._nddStore);
     await Promise.all(fileNames.map(async fileName => {
       const match = fileName.match(/([0-9]+)-ready/);
-      if (match) {
-        const [_, id] = match;
-        process.kill(id, 'SIGKILL');
-      }
-    }));
-    await this.cleanup();
-    await this.dispose();
-  }
-
-  async cleanup() {
-    const fileNames = await util.promisify(fs.readdir)(this._nddStore);
-    await Promise.all(fileNames.map(async fileName => {
-      const match = fileName.match(/([0-9]+)-ready/);
-      if (match) {
-        const [_, id] = match;
-        let exists = true;
-        try {
-          process.kill(id, 0);
-        } catch (e) {
-          exists = false;
-        }
-        if (!exists)
-          await util.promisify(fs.unlink)(path.join(this._nddStore, fileName)).catch(() => 0);
-      }
+      if (match)
+        process.kill(match[1], 'SIGKILL');
     }));
-  }
-
-  dispose() {
-    this._updateScheduled.clear();
     this._instances.clear();
     if (this._nddStoreWatcher) {
-      this._nddStoreWatcher.removeListener('change', this._watchEventListener);
       this._nddStoreWatcher.close();
+      this._nddStoreWatcher = null;
     }
   }
 
-  async _fetchExisting() {
-    const fileNames = await util.promisify(fs.readdir)(this._nddStore);
-    for (const fileName of fileNames)
-      this._onWatchEvent('rename', fileName);
-  }
-
-  /**
-   * @param {string} eventType
-   * @param {string} fileName
-   */
-  _onWatchEvent(eventType, fileName) {
-    if (eventType === 'rename') {
-      const match = fileName.match(/([0-9]+)-ready/);
-      if (match) {
-        const [_, id] = match;
-        this._scheduleUpdate(id);
-      }
-    }
-  }
-
-  /**
-   * @param {string} id
-   */
-  _scheduleUpdate(id) {
-    if (!this._updateScheduled.has(id)) {
-      setImmediate(() => this._update(id));
-      this._updateScheduled.add(id);
-    }
-  }
-
-  /**
-   * @param {string} id
-   */
-  async _update(id) {
-    this._updateScheduled.delete(id);
-
-    const files = await util.promisify(fs.readdir)(this._nddStore);
-    const re = new RegExp(`${id}-ready`);
-    let isFinished = true;
-    for (const file of files) {
-      const match = file.match(re);
-      if (match) {
-        isFinished = false;
-        break;
-      }
-    }
-    if (this._instances.has(id) && isFinished) {
-      const instance = this._instances.get(id);
-      instance._finished();
-      this._instances.delete(id);
-      this.emit('finished', instance);
-    } else if (!isFinished) {
-      const instance = new NodeProcess(id, this._nddStore);
-      this._instances.set(id, instance);
-      this.emit('added', instance);
-    }
+  detach(id) {
+    const name = path.join(this._nddStore, `${id}-ready`);
+    if (fs.existsSync(name))
+      util.promisify(fs.unlink)(name);
   }
 }
 
@@ -139,16 +87,6 @@ class NodeProcess {
 
     this._id = id;
     this._info = null;
-    this._isFinished = false;
-
-    this._finishedCallback = null;
-  }
-
-  /**
-   * @return {boolean}
-   */
-  isFinished() {
-    return this._isFinished;
   }
 
   /**
@@ -158,17 +96,6 @@ class NodeProcess {
     return this._id;
   }
 
-  async kill() {
-    if (!this._isFinished) {
-      const done = new Promise(resolve => this._finishedCallback = resolve);
-      try {
-        process.kill(this._id, 'SIGKILL');
-      } catch (e) {
-      }
-      await done;
-    }
-  }
-
   /**
    * @return {!Promise<?Object>}
    */
@@ -185,10 +112,11 @@ class NodeProcess {
     return this._info;
   }
 
-  _finished() {
-    this._isFinished = true;
-    if (this._finishedCallback)
-      this._finishedCallback();
+  kill() {
+    try {
+      process.kill(Number(this._id), 'SIGKILL');
+    } catch (e) {
+    }
   }
 }
 

diff --git a/node_debug_demon/preload.js b/node_debug_demon/preload.js
@@ -7,13 +7,14 @@
 try {
   const fs = require('fs');
   const path = require('path');
+
   const nddStore = process.env.NDD_STORE;
   const nddWaitAtStart = !!process.env.NDD_WAIT_AT_START || false;
   const nodePid = process.pid;
   const nddGroupId = process.env.NDD_GROUP_ID || `${nodePid}:${Date.now()}`;
   if (process.env.NDD_GROUP_ID !== nddGroupId)
     process.env.NDD_GROUP_ID = nddGroupId;
-  const parentId = process.env.NDD_PID || undefined;
+  const parentProcessId = process.env.NDD_PID;
   process.env.NDD_PID = process.pid;
 
   const stateFileName = path.join(nddStore, `${nodePid}`);
@@ -23,8 +24,8 @@ try {
     argv: process.argv.concat(process.execArgv),
     waitAtStart: nddWaitAtStart
   };
-  if (parentId)
-    state.parentId = parentId;
+  if (parentProcessId)
+    state.parentId = parentProcessId;
   fs.writeFileSync(stateFileName, JSON.stringify(state));
 
   const readyFileName = path.join(nddStore, `${nodePid}-ready`);
@@ -45,20 +46,15 @@ try {
     const buffer = [];
     const store = message => buffer.push(message);
     process.on('message', store);
-    require(process.env.NDD_DEASYNC_JS).loopWhile(() => wait);
+
+    while (wait) require(process.env.NDD_DEASYNC_JS).sleep(100);
+
     process.removeListener('message', store);
     if (buffer.length)
       setTimeout(_ => buffer.map(message => process.emit('message', message)), 0);
     delete process.runIfWaitingAtStart;
   } else {
     fs.renameSync(stateFileName, readyFileName);
   }
-  process.on('exit', _ => {
-    try {
-      fs.unlinkSync(readyFileName);
-    } catch (e) {
-      console.log(e.stack);
-    }
-  });
 } catch (e) {
 }
diff --git a/services/ndd_service.js b/services/ndd_service.js
@@ -22,31 +22,21 @@ class NddService extends ServiceBase {
   constructor() {
     super();
     this._nddStore = '';
-    this._useTemporaryNddStore = false;
     this._nddClient = null;
     this._onAddedListener = this._onAdded.bind(this);
     this._onFinishedListener = this._onFinished.bind(this);
 
-    this._cleanupInterval = null;
-
     this._instances = new Map();
   }
 
-  async start({nddStore, cleanupInterval}) {
+  async start() {
     if (this._nddClient)
       return;
-    this._useTemporaryNddStore = !nddStore;
-    this._nddStore = nddStore || await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'ndb-'));
+    this._nddStore = await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'ndb-'));
     this._nddClient = new Client(this._nddStore);
     this._nddClient.on('added', this._onAddedListener);
     this._nddClient.on('finished', this._onFinishedListener);
     await this._nddClient.start();
-    this._cleanupInterval = setInterval(() => this._nddClient.cleanup(), cleanupInterval || 1000);
-  }
-
-  async nddStore() {
-    if (!this._nddClient)
-      throw 'NddService should be started first';
     return this._nddStore;
   }
 
@@ -57,16 +47,15 @@ class NddService extends ServiceBase {
     this._nddClient.removeListener('added', this._onAddedListener);
     this._nddClient.removeListener('finished', this._onFinishedListener);
     this._instances.clear();
-    if (this._useTemporaryNddStore)
-      await util.promisify(removeFolder)(this._nddStore);
-
-    if (this._cleanupInterval)
-      clearInterval(this._cleanupInterval);
+    await util.promisify(removeFolder)(this._nddStore);
   }
 
   async dispose() {
-    await this.stop();
-    process.exit(0);
+    try {
+      await this.stop();
+    } finally {
+      process.exit(0);
+    }
   }
 
   async attach({instanceId}) {
@@ -88,6 +77,7 @@ class NddService extends ServiceBase {
         instanceId: instance.id()
       });
       delete instance[wsSymbol];
+      this._nddClient.detach(instance.id());
     });
     ws.on('open', _ => {
       instance[wsSymbol] = ws;
@@ -167,7 +157,7 @@ class NddService extends ServiceBase {
     const instance = this._instances.get(instanceId);
     if (!instance)
       throw 'No instance with given id';
-    await instance.kill();
+    instance.kill();
   }
 
   async _onAdded(instance) {