Skip to content

Commit c04ad14

Browse files
dgozmanJoelEinbinder
authored andcommitted
feat(launcher): gracefully close browser on sigint (#650)
1 parent 3248749 commit c04ad14

File tree

7 files changed

+150
-47
lines changed

7 files changed

+150
-47
lines changed

src/server/chromium.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,9 +116,9 @@ export class Chromium implements BrowserType {
116116
const message = { method: 'Browser.close', id: kBrowserCloseMessageId };
117117
transport.send(JSON.stringify(message));
118118
},
119-
onkill: () => {
119+
onkill: (exitCode, signal) => {
120120
if (browserApp)
121-
browserApp.emit(Events.BrowserApp.Close);
121+
browserApp.emit(Events.BrowserApp.Close, exitCode, signal);
122122
},
123123
});
124124

src/server/firefox.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,9 @@ export class Firefox implements BrowserType {
115115
const message = { method: 'Browser.close', params: {}, id: kBrowserCloseMessageId };
116116
transport.send(JSON.stringify(message));
117117
},
118-
onkill: () => {
118+
onkill: (exitCode, signal) => {
119119
if (browserApp)
120-
browserApp.emit(Events.BrowserApp.Close);
120+
browserApp.emit(Events.BrowserApp.Close, exitCode, signal);
121121
},
122122
});
123123

src/server/processLauncher.ts

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ export type LaunchProcessOptions = {
4040

4141
// Note: attemptToGracefullyClose should reject if it does not close the browser.
4242
attemptToGracefullyClose: () => Promise<any>,
43-
onkill: () => void,
43+
onkill: (exitCode: number | null, signal: string | null) => void,
4444
};
4545

4646
type LaunchResult = { launchedProcess: childProcess.ChildProcess, gracefullyClose: () => Promise<void> };
@@ -86,10 +86,11 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
8686

8787
let processClosed = false;
8888
const waitForProcessToClose = new Promise((fulfill, reject) => {
89-
spawnedProcess.once('exit', () => {
89+
spawnedProcess.once('exit', (exitCode, signal) => {
9090
debugLauncher(`[${id}] <process did exit>`);
9191
processClosed = true;
9292
helper.removeEventListeners(listeners);
93+
options.onkill(exitCode, signal);
9394
// Cleanup as processes exit.
9495
if (options.tempDir) {
9596
removeFolderAsync(options.tempDir)
@@ -98,33 +99,36 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
9899
} else {
99100
fulfill();
100101
}
101-
options.onkill();
102102
});
103103
});
104104

105105
const listeners = [ helper.addEventListener(process, 'exit', killProcess) ];
106-
if (options.handleSIGINT)
107-
listeners.push(helper.addEventListener(process, 'SIGINT', () => { killProcess(); process.exit(130); }));
106+
if (options.handleSIGINT) {
107+
listeners.push(helper.addEventListener(process, 'SIGINT', () => {
108+
gracefullyClose().then(() => process.exit(130));
109+
}));
110+
}
108111
if (options.handleSIGTERM)
109112
listeners.push(helper.addEventListener(process, 'SIGTERM', gracefullyClose));
110113
if (options.handleSIGHUP)
111114
listeners.push(helper.addEventListener(process, 'SIGHUP', gracefullyClose));
112-
let gracefullyClosing = false;
113-
return { launchedProcess: spawnedProcess, gracefullyClose };
114115

116+
let gracefullyClosing = false;
115117
async function gracefullyClose(): Promise<void> {
116118
// We keep listeners until we are done, to handle 'exit' and 'SIGINT' while
117119
// asynchronously closing to prevent zombie processes. This might introduce
118-
// reentrancy to this function.
119-
if (gracefullyClosing)
120+
// reentrancy to this function, for example user sends SIGINT second time.
121+
// In this case, let's forcefully kill the process.
122+
if (gracefullyClosing) {
123+
debugLauncher(`[${id}] <forecefully close>`);
124+
killProcess();
120125
return;
126+
}
121127
gracefullyClosing = true;
122128
debugLauncher(`[${id}] <gracefully close start>`);
123129
options.attemptToGracefullyClose().catch(() => killProcess());
124-
// TODO: forcefully kill the process after some timeout.
125130
await waitForProcessToClose;
126131
debugLauncher(`[${id}] <gracefully close end>`);
127-
helper.removeEventListeners(listeners);
128132
}
129133

130134
// This method has to be sync to be used as 'exit' event handler.
@@ -148,6 +152,8 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
148152
removeFolder.sync(options.tempDir);
149153
} catch (e) { }
150154
}
155+
156+
return { launchedProcess: spawnedProcess, gracefullyClose };
151157
}
152158

153159
export function waitForLine(process: childProcess.ChildProcess, inputStream: stream.Readable, regex: RegExp, timeout: number, timeoutError: TimeoutError): Promise<RegExpMatchArray> {

src/server/webkit.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,9 @@ export class WebKit implements BrowserType {
117117
const message = JSON.stringify({method: 'Browser.close', params: {}, id: kBrowserCloseMessageId});
118118
transport.send(message);
119119
},
120-
onkill: () => {
120+
onkill: (exitCode, signal) => {
121121
if (browserApp)
122-
browserApp.emit(Events.BrowserApp.Close);
122+
browserApp.emit(Events.BrowserApp.Close, exitCode, signal);
123123
},
124124
});
125125

test/fixtures.spec.js

Lines changed: 122 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,56 @@
1616
*/
1717

1818
const path = require('path');
19-
const {spawn} = require('child_process');
19+
const {spawn, execSync} = require('child_process');
2020

21-
module.exports.describe = function({testRunner, expect, product, playwrightPath, FFOX, CHROMIUM, WEBKIT}) {
21+
module.exports.describe = function({testRunner, expect, product, playwright, playwrightPath, defaultBrowserOptions, WIN, FFOX, CHROMIUM, WEBKIT}) {
2222
const {describe, xdescribe, fdescribe} = testRunner;
2323
const {it, fit, xit, dit} = testRunner;
2424
const {beforeAll, beforeEach, afterAll, afterEach} = testRunner;
2525

26+
async function testSignal(action) {
27+
const options = Object.assign({}, defaultBrowserOptions, {
28+
// Disable DUMPIO to cleanly read stdout.
29+
dumpio: false,
30+
webSocket: true,
31+
handleSIGINT: true,
32+
handleSIGTERM: true,
33+
handleSIGHUP: true,
34+
});
35+
const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), playwrightPath, product, JSON.stringify(options)]);
36+
let wsEndPointCallback;
37+
const wsEndPointPromise = new Promise(x => wsEndPointCallback = x);
38+
let output = '';
39+
let browserExitCode = 'none';
40+
let browserSignal = 'none';
41+
let browserPid;
42+
res.stdout.on('data', data => {
43+
output += data;
44+
// Uncomment to debug these tests.
45+
// console.log(data.toString());
46+
let match = output.match(/browserWS:(.+):browserWS/);
47+
if (match)
48+
wsEndPointCallback(match[1]);
49+
match = output.match(/browserClose:([^:]+):([^:]+):browserClose/);
50+
if (match) {
51+
browserExitCode = match[1];
52+
browserSignal = match[2];
53+
}
54+
match = output.match(/browserPid:([^:]+):browserPid/);
55+
if (match)
56+
browserPid = +match[1];
57+
});
58+
res.on('error', (...args) => console.log("ERROR", ...args));
59+
const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise });
60+
const promises = [
61+
new Promise(resolve => browser.once('disconnected', resolve)),
62+
new Promise(resolve => res.on('exit', resolve)),
63+
];
64+
action(res, browserPid);
65+
const [, exitCode] = await Promise.all(promises);
66+
return { exitCode, browserSignal, browserExitCode, output };
67+
}
68+
2669
describe('Fixtures', function() {
2770
it('dumpio option should work with webSocket option', async({server}) => {
2871
let dumpioData = '';
@@ -38,5 +81,82 @@ module.exports.describe = function({testRunner, expect, product, playwrightPath,
3881
await new Promise(resolve => res.on('close', resolve));
3982
expect(dumpioData).toContain('message from dumpio');
4083
});
84+
it('should close the browser when the node process closes', async () => {
85+
const result = await testSignal(child => {
86+
if (process.platform === 'win32')
87+
execSync(`taskkill /pid ${child.pid} /T /F`);
88+
else
89+
process.kill(child.pid);
90+
});
91+
expect(result.exitCode).toBe(WIN ? 1 : 0);
92+
// We might not get browser exitCode in time when killing the parent node process,
93+
// so we don't check it here.
94+
});
95+
if (!WIN) {
96+
// Cannot reliably send signals on Windows.
97+
it('should report browser close signal', async () => {
98+
const result = await testSignal((child, browserPid) => {
99+
process.kill(browserPid);
100+
process.kill(child.pid, 'SIGINT');
101+
});
102+
expect(result.exitCode).toBe(130);
103+
expect(result.browserExitCode).toBe('null');
104+
expect(result.browserSignal).toBe('SIGTERM');
105+
});
106+
it('should report browser close signal 2', async () => {
107+
const result = await testSignal((child, browserPid) => {
108+
process.kill(browserPid, 'SIGKILL');
109+
process.kill(child.pid, 'SIGINT');
110+
});
111+
expect(result.exitCode).toBe(130);
112+
expect(result.browserExitCode).toBe('null');
113+
expect(result.browserSignal).toBe('SIGKILL');
114+
});
115+
it('should close the browser on SIGINT', async () => {
116+
const result = await testSignal(child => process.kill(child.pid, 'SIGINT'));
117+
expect(result.exitCode).toBe(130);
118+
expect(result.browserExitCode).toBe('0');
119+
expect(result.browserSignal).toBe('null');
120+
});
121+
it('should close the browser on SIGTERM', async () => {
122+
const result = await testSignal(child => process.kill(child.pid, 'SIGTERM'));
123+
expect(result.exitCode).toBe(0);
124+
expect(result.browserExitCode).toBe('0');
125+
expect(result.browserSignal).toBe('null');
126+
});
127+
it('should close the browser on SIGHUP', async () => {
128+
const result = await testSignal(child => process.kill(child.pid, 'SIGHUP'));
129+
expect(result.exitCode).toBe(0);
130+
expect(result.browserExitCode).toBe('0');
131+
expect(result.browserSignal).toBe('null');
132+
});
133+
it('should kill the browser on double SIGINT', async () => {
134+
const result = await testSignal(child => {
135+
process.kill(child.pid, 'SIGINT');
136+
process.kill(child.pid, 'SIGINT');
137+
});
138+
expect(result.exitCode).toBe(130);
139+
// TODO: ideally, we would expect the SIGKILL on the browser from
140+
// force kill, but that's racy with sending two signals.
141+
});
142+
it('should kill the browser on SIGINT + SIGTERM', async () => {
143+
const result = await testSignal(child => {
144+
process.kill(child.pid, 'SIGINT');
145+
process.kill(child.pid, 'SIGTERM');
146+
});
147+
expect(result.exitCode).toBe(130);
148+
// TODO: ideally, we would expect the SIGKILL on the browser from
149+
// force kill, but that's racy with sending two signals.
150+
});
151+
it('should kill the browser on SIGTERM + SIGINT', async () => {
152+
const result = await testSignal(child => {
153+
process.kill(child.pid, 'SIGTERM');
154+
process.kill(child.pid, 'SIGINT');
155+
});
156+
expect(result.exitCode).toBe(130);
157+
// TODO: ideally, we would expect the SIGKILL on the browser from
158+
// force kill, but that's racy with sending two signals.
159+
});
160+
}
41161
});
42162
};

test/fixtures/closeme.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
(async() => {
22
const [, , playwrightRoot, product, options] = process.argv;
33
const browserApp = await require(playwrightRoot)[product.toLowerCase()].launchBrowserApp(JSON.parse(options));
4-
console.log(browserApp.wsEndpoint());
4+
browserApp.on('close', (exitCode, signal) => {
5+
console.log(`browserClose:${exitCode}:${signal}:browserClose`);
6+
});
7+
console.log(`browserPid:${browserApp.process().pid}:browserPid`);
8+
console.log(`browserWS:${browserApp.wsEndpoint()}:browserWS`);
59
})();

test/launcher.spec.js

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ const fs = require('fs');
1818
const os = require('os');
1919
const path = require('path');
2020
const util = require('util');
21-
const {spawn, execSync} = require('child_process');
2221

2322
const utils = require('./utils');
2423
const rmAsync = util.promisify(require('rimraf'));
@@ -67,32 +66,6 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p
6766
expect(page.url()).toBe(server.EMPTY_PAGE);
6867
await browser.close();
6968
});
70-
it('should close the browser when the node process closes', async({ server }) => {
71-
const options = Object.assign({}, defaultBrowserOptions, {
72-
// Disable DUMPIO to cleanly read stdout.
73-
dumpio: false,
74-
webSocket: true,
75-
});
76-
const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), playwrightPath, product, JSON.stringify(options)]);
77-
let wsEndPointCallback;
78-
const wsEndPointPromise = new Promise(x => wsEndPointCallback = x);
79-
let output = '';
80-
res.stdout.on('data', data => {
81-
output += data;
82-
if (output.indexOf('\n'))
83-
wsEndPointCallback(output.substring(0, output.indexOf('\n')));
84-
});
85-
const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise });
86-
const promises = [
87-
new Promise(resolve => browser.once('disconnected', resolve)),
88-
new Promise(resolve => res.on('exit', resolve))
89-
];
90-
if (process.platform === 'win32')
91-
execSync(`taskkill /pid ${res.pid} /T /F`);
92-
else
93-
process.kill(res.pid);
94-
await Promise.all(promises);
95-
});
9669
it('should return child_process instance', async () => {
9770
const browserApp = await playwright.launchBrowserApp(defaultBrowserOptions);
9871
expect(browserApp.process().pid).toBeGreaterThan(0);

0 commit comments

Comments
 (0)