Skip to content

Commit

Permalink
make launcher.kill() synchronous (#268)
Browse files Browse the repository at this point in the history
  • Loading branch information
@paulirish
paulirish authored May 31, 2022
1 parent 3561350 commit 3724165
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 59 deletions.
96 changes: 46 additions & 50 deletions src/chrome-launcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ export interface ModuleOverrides {
spawn?: typeof childProcess.spawn;
}

const sigintListener = async () => {
await killAll();
const sigintListener = () => {
killAll();
process.exit(_SIGINT_EXIT_CODE);
};

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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, maxRetries: 10});
}
};

Expand Down
39 changes: 30 additions & 9 deletions test/chrome-launcher-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}) => {
Expand Down Expand Up @@ -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});

Expand All @@ -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});

Expand All @@ -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});

Expand All @@ -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});

Expand Down

0 comments on commit 3724165

Please sign in to comment.