make launcher.kill() synchronous, fs.rm now gets force and maxRetries

src/chrome-launcher.ts

@@ -54,8 +54,8 @@ export interface ModuleOverrides {
   spawn?: typeof childProcess.spawn;
 }
 
-const sigintListener = async () => {
-  await killAll();
+const sigintListener = () => {
+  killAll();
   process.exit(_SIGINT_EXIT_CODE);
 };
 
@@ -92,11 +92,11 @@ function getChromePath(): string {
   return installation;
 }
 
-async function killAll(): Promise<Array<Error>> {
+function killAll(): Array<Error> {
   let errors = [];
   for (const instance of instances) {
     try {
-      await instance.kill();
+      instance.kill();
       // only delete if kill did not error
       // this means erroring instances remain in the Set
       instances.delete(instance);
@@ -380,60 +380,56 @@ class Launcher {
   }
 
   kill() {
-    return new Promise<void>((resolve) => {
-      if (this.chromeProcess) {
-        this.chromeProcess.on('close', () => {
-          delete this.chromeProcess;
-          this.destroyTmp().then(resolve);
-        });
-
-        log.log('ChromeLauncher', `Killing Chrome instance ${this.chromeProcess.pid}`);
-        try {
-          if (isWindows) {
-            // https://github.com/GoogleChrome/chrome-launcher/issues/266
-            const taskkillProc = spawnSync(
-                `taskkill /pid ${this.chromeProcess.pid} /T /F`, {shell: true, encoding: 'utf-8'});
-
-            const {stderr} = taskkillProc;
-            if (stderr) log.error('ChromeLauncher', `taskkill stderr`, stderr);
-          } else {
-            if (this.chromeProcess.pid) {
-              process.kill(-this.chromeProcess.pid, 'SIGKILL');
-            }
-          }
-        } catch (err) {
-          const message = `Chrome could not be killed ${err.message}`;
-          log.warn('ChromeLauncher', message);
-        }
+    if (!this.chromeProcess) {
+      return;
+    }
+
+    this.chromeProcess.on('close', () => {
+      delete this.chromeProcess;
+      this.destroyTmp();
+    });
+
+    log.log('ChromeLauncher', `Killing Chrome instance ${this.chromeProcess.pid}`);
+    try {
+      if (isWindows) {
+        // https://github.com/GoogleChrome/chrome-launcher/issues/266
+        const taskkillProc = spawnSync(
+            `taskkill /pid ${this.chromeProcess.pid} /T /F`, {shell: true, encoding: 'utf-8'});
+
+        const {stderr} = taskkillProc;
+        if (stderr) log.error('ChromeLauncher', `taskkill stderr`, stderr);
       } else {
-        // fail silently as we did not start chrome
-        resolve();
+        if (this.chromeProcess.pid) {
+          process.kill(-this.chromeProcess.pid, 'SIGKILL');
+        }
       }
-    });
+    } catch (err) {
+      const message = `Chrome could not be killed ${err.message}`;
+      log.warn('ChromeLauncher', message);
+    }
+    this.destroyTmp();
   }
 
   destroyTmp() {
-    return new Promise<void>(resolve => {
-      // Only clean up the tmp dir if we created it.
-      if (this.userDataDir === undefined || this.opts.userDataDir !== undefined) {
-        return resolve();
-      }
+    // Only clean up the tmp dir if we created it.
+    if (this.userDataDir === undefined || this.opts.userDataDir !== undefined) {
+      return;
+    }
 
-      if (this.outFile) {
-        this.fs.closeSync(this.outFile);
-        delete this.outFile;
-      }
+    if (this.outFile) {
+      this.fs.closeSync(this.outFile);
+      delete this.outFile;
+    }
 
-      if (this.errFile) {
-        this.fs.closeSync(this.errFile);
-        delete this.errFile;
-      }
+    if (this.errFile) {
+      this.fs.closeSync(this.errFile);
+      delete this.errFile;
+    }
 
-      // backwards support for node v12 + v14.14+
-      // https://nodejs.org/api/deprecations.html#DEP0147
-      const rm = this.fs.rm || this.fs.rmdir;
-      rm(this.userDataDir, {recursive: true}, () => resolve());
-    });
+    // backwards support for node v12 + v14.14+
+    // https://nodejs.org/api/deprecations.html#DEP0147
+    const rmSync = this.fs.rmSync || this.fs.rmdirSync;
+    rmSync(this.userDataDir, {recursive: true, force: true});
   }
 };
 

test/chrome-launcher-test.ts

@@ -10,13 +10,15 @@ import {DEFAULT_FLAGS} from '../src/flags';
 
 import {spy, stub} from 'sinon';
 import * as assert from 'assert';
+import * as fs from 'fs';
 
 const log = require('lighthouse-logger');
 const fsMock = {
   openSync: () => {},
   closeSync: () => {},
   writeFileSync: () => {},
-  rmdir: () => {},
+  rmdirSync: () => {},
+  rmSync: () => {},
 };
 
 const launchChromeWithOpts = async (opts: Options = {}) => {
@@ -54,22 +56,22 @@ describe('Launcher', () => {
   });
 
   it('accepts and uses a custom path', async () => {
-    const fs = {...fsMock, rmdir: spy(), rm: spy()};
+    const fs = {...fsMock, rmdirSync: spy(), rmSync: spy()};
     const chromeInstance =
         new Launcher({userDataDir: 'some_path'}, {fs: fs as any});
 
     chromeInstance.prepare();
 
     await chromeInstance.destroyTmp();
-    assert.strictEqual(fs.rmdir.callCount, 0);
-    assert.strictEqual(fs.rm.callCount, 0);
+    assert.strictEqual(fs.rmdirSync.callCount, 0);
+    assert.strictEqual(fs.rmSync.callCount, 0);
   });
 
   it('allows to overwrite browser prefs', async () => {
     const existStub = stub().returns(true)
     const readFileStub = stub().returns(JSON.stringify({ some: 'prefs' }))
     const writeFileStub = stub()
-    const fs = {...fsMock, rmdir: spy(), readFileSync: readFileStub, writeFileSync: writeFileStub, existsSync: existStub };
+    const fs = {...fsMock, rmdirSync: spy(), readFileSync: readFileStub, writeFileSync: writeFileStub, existsSync: existStub };
     const chromeInstance =
         new Launcher({prefs: {'download.default_directory': '/some/dir'}}, {fs: fs as any});
 
@@ -84,7 +86,7 @@ describe('Launcher', () => {
     const existStub = stub().returns(false)
     const readFileStub = stub().returns(Buffer.from(JSON.stringify({ some: 'prefs' })))
     const writeFileStub = stub()
-    const fs = {...fsMock, rmdir: spy(), readFileSync: readFileStub, writeFileSync: writeFileStub, existsSync: existStub };
+    const fs = {...fsMock, rmdirSync: spy(), readFileSync: readFileStub, writeFileSync: writeFileStub, existsSync: existStub };
     const chromeInstance =
         new Launcher({prefs: {'download.default_directory': '/some/dir'}}, {fs: fs as any});
 
@@ -96,9 +98,9 @@ describe('Launcher', () => {
     )
   });
 
-  it('cleans up the tmp dir after closing', async () => {
-    const rmMock = stub().callsFake((_path, _options, done) => done());
-    const fs = {...fsMock, rmdir: rmMock, rm: rmMock};
+  it('cleans up the tmp dir after closing (mocked)', async () => {
+    const rmMock = stub().callsFake((_path, _options) => {});
+    const fs = {...fsMock, rmdirSync: rmMock, rmSync: rmMock};
 
     const chromeInstance = new Launcher({}, {fs: fs as any});
 
@@ -107,6 +109,25 @@ describe('Launcher', () => {
     assert.strictEqual(rmMock.callCount, 1);
   });
 
+
+  it('cleans up the tmp dir after closing (real)', async () => {
+    const rmSpy = spy(fs, 'rmSync' in fs ? 'rmSync' : 'rmdirSync');
+    const fsFake = {...fsMock, rmdirSync: rmSpy, rmSync: rmSpy};
+
+    const chromeInstance = new Launcher({}, {fs: fsFake as any});
+
+    await chromeInstance.launch();
+    assert.ok(chromeInstance.userDataDir);
+    assert.ok(fs.existsSync(chromeInstance.userDataDir));
+
+    chromeInstance.kill();
+
+    // tmpdir is gone 
+    const [path] = fsFake.rmSync.getCall(0).args;
+    assert.strictEqual(chromeInstance.userDataDir, path);
+    assert.equal(fs.existsSync(path), false, `userdatadir still exists: ${path}`);
+  }).timeout(30 * 1000);
+
   it('does not delete created directory when custom path passed', () => {
     const chromeInstance = new Launcher({userDataDir: 'some_path'}, {fs: fsMock as any});