diff --git a/src/server/chromium.ts b/src/server/chromium.ts index 8b0736e4ae..7f2ad85f22 100644 --- a/src/server/chromium.ts +++ b/src/server/chromium.ts @@ -116,9 +116,9 @@ export class Chromium implements BrowserType { const message = { method: 'Browser.close', id: kBrowserCloseMessageId }; transport.send(JSON.stringify(message)); }, - onkill: () => { + onkill: (exitCode, signal) => { if (browserApp) - browserApp.emit(Events.BrowserApp.Close); + browserApp.emit(Events.BrowserApp.Close, exitCode, signal); }, }); diff --git a/src/server/firefox.ts b/src/server/firefox.ts index fa553dd1ca..2769b472b8 100644 --- a/src/server/firefox.ts +++ b/src/server/firefox.ts @@ -115,9 +115,9 @@ export class Firefox implements BrowserType { const message = { method: 'Browser.close', params: {}, id: kBrowserCloseMessageId }; transport.send(JSON.stringify(message)); }, - onkill: () => { + onkill: (exitCode, signal) => { if (browserApp) - browserApp.emit(Events.BrowserApp.Close); + browserApp.emit(Events.BrowserApp.Close, exitCode, signal); }, }); diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts index f9526d9f08..99ad26dc48 100644 --- a/src/server/processLauncher.ts +++ b/src/server/processLauncher.ts @@ -40,7 +40,7 @@ export type LaunchProcessOptions = { // Note: attemptToGracefullyClose should reject if it does not close the browser. attemptToGracefullyClose: () => Promise, - onkill: () => void, + onkill: (exitCode: number | null, signal: string | null) => void, }; type LaunchResult = { launchedProcess: childProcess.ChildProcess, gracefullyClose: () => Promise }; @@ -86,10 +86,11 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { - spawnedProcess.once('exit', () => { + spawnedProcess.once('exit', (exitCode, signal) => { debugLauncher(`[${id}] `); processClosed = true; helper.removeEventListeners(listeners); + options.onkill(exitCode, signal); // Cleanup as processes exit. if (options.tempDir) { removeFolderAsync(options.tempDir) @@ -98,33 +99,36 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { killProcess(); process.exit(130); })); + if (options.handleSIGINT) { + listeners.push(helper.addEventListener(process, 'SIGINT', () => { + gracefullyClose().then(() => process.exit(130)); + })); + } if (options.handleSIGTERM) listeners.push(helper.addEventListener(process, 'SIGTERM', gracefullyClose)); if (options.handleSIGHUP) listeners.push(helper.addEventListener(process, 'SIGHUP', gracefullyClose)); - let gracefullyClosing = false; - return { launchedProcess: spawnedProcess, gracefullyClose }; + let gracefullyClosing = false; async function gracefullyClose(): Promise { // We keep listeners until we are done, to handle 'exit' and 'SIGINT' while // asynchronously closing to prevent zombie processes. This might introduce - // reentrancy to this function. - if (gracefullyClosing) + // reentrancy to this function, for example user sends SIGINT second time. + // In this case, let's forcefully kill the process. + if (gracefullyClosing) { + debugLauncher(`[${id}] `); + killProcess(); return; + } gracefullyClosing = true; debugLauncher(`[${id}] `); options.attemptToGracefullyClose().catch(() => killProcess()); - // TODO: forcefully kill the process after some timeout. await waitForProcessToClose; debugLauncher(`[${id}] `); - helper.removeEventListeners(listeners); } // This method has to be sync to be used as 'exit' event handler. @@ -148,6 +152,8 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { diff --git a/src/server/webkit.ts b/src/server/webkit.ts index 9d6a8cc55d..23bb49f620 100644 --- a/src/server/webkit.ts +++ b/src/server/webkit.ts @@ -117,9 +117,9 @@ export class WebKit implements BrowserType { const message = JSON.stringify({method: 'Browser.close', params: {}, id: kBrowserCloseMessageId}); transport.send(message); }, - onkill: () => { + onkill: (exitCode, signal) => { if (browserApp) - browserApp.emit(Events.BrowserApp.Close); + browserApp.emit(Events.BrowserApp.Close, exitCode, signal); }, }); diff --git a/test/fixtures.spec.js b/test/fixtures.spec.js index df3601fd07..c88bd66913 100644 --- a/test/fixtures.spec.js +++ b/test/fixtures.spec.js @@ -16,13 +16,56 @@ */ const path = require('path'); -const {spawn} = require('child_process'); +const {spawn, execSync} = require('child_process'); -module.exports.describe = function({testRunner, expect, product, playwrightPath, FFOX, CHROMIUM, WEBKIT}) { +module.exports.describe = function({testRunner, expect, product, playwright, playwrightPath, defaultBrowserOptions, WIN, FFOX, CHROMIUM, WEBKIT}) { const {describe, xdescribe, fdescribe} = testRunner; const {it, fit, xit, dit} = testRunner; const {beforeAll, beforeEach, afterAll, afterEach} = testRunner; + async function testSignal(action) { + const options = Object.assign({}, defaultBrowserOptions, { + // Disable DUMPIO to cleanly read stdout. + dumpio: false, + webSocket: true, + handleSIGINT: true, + handleSIGTERM: true, + handleSIGHUP: true, + }); + const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), playwrightPath, product, JSON.stringify(options)]); + let wsEndPointCallback; + const wsEndPointPromise = new Promise(x => wsEndPointCallback = x); + let output = ''; + let browserExitCode = 'none'; + let browserSignal = 'none'; + let browserPid; + res.stdout.on('data', data => { + output += data; + // Uncomment to debug these tests. + // console.log(data.toString()); + let match = output.match(/browserWS:(.+):browserWS/); + if (match) + wsEndPointCallback(match[1]); + match = output.match(/browserClose:([^:]+):([^:]+):browserClose/); + if (match) { + browserExitCode = match[1]; + browserSignal = match[2]; + } + match = output.match(/browserPid:([^:]+):browserPid/); + if (match) + browserPid = +match[1]; + }); + res.on('error', (...args) => console.log("ERROR", ...args)); + const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise }); + const promises = [ + new Promise(resolve => browser.once('disconnected', resolve)), + new Promise(resolve => res.on('exit', resolve)), + ]; + action(res, browserPid); + const [, exitCode] = await Promise.all(promises); + return { exitCode, browserSignal, browserExitCode, output }; + } + describe('Fixtures', function() { it('dumpio option should work with webSocket option', async({server}) => { let dumpioData = ''; @@ -38,5 +81,82 @@ module.exports.describe = function({testRunner, expect, product, playwrightPath, await new Promise(resolve => res.on('close', resolve)); expect(dumpioData).toContain('message from dumpio'); }); + it('should close the browser when the node process closes', async () => { + const result = await testSignal(child => { + if (process.platform === 'win32') + execSync(`taskkill /pid ${child.pid} /T /F`); + else + process.kill(child.pid); + }); + expect(result.exitCode).toBe(WIN ? 1 : 0); + // We might not get browser exitCode in time when killing the parent node process, + // so we don't check it here. + }); + if (!WIN) { + // Cannot reliably send signals on Windows. + it('should report browser close signal', async () => { + const result = await testSignal((child, browserPid) => { + process.kill(browserPid); + process.kill(child.pid, 'SIGINT'); + }); + expect(result.exitCode).toBe(130); + expect(result.browserExitCode).toBe('null'); + expect(result.browserSignal).toBe('SIGTERM'); + }); + it('should report browser close signal 2', async () => { + const result = await testSignal((child, browserPid) => { + process.kill(browserPid, 'SIGKILL'); + process.kill(child.pid, 'SIGINT'); + }); + expect(result.exitCode).toBe(130); + expect(result.browserExitCode).toBe('null'); + expect(result.browserSignal).toBe('SIGKILL'); + }); + it('should close the browser on SIGINT', async () => { + const result = await testSignal(child => process.kill(child.pid, 'SIGINT')); + expect(result.exitCode).toBe(130); + expect(result.browserExitCode).toBe('0'); + expect(result.browserSignal).toBe('null'); + }); + it('should close the browser on SIGTERM', async () => { + const result = await testSignal(child => process.kill(child.pid, 'SIGTERM')); + expect(result.exitCode).toBe(0); + expect(result.browserExitCode).toBe('0'); + expect(result.browserSignal).toBe('null'); + }); + it('should close the browser on SIGHUP', async () => { + const result = await testSignal(child => process.kill(child.pid, 'SIGHUP')); + expect(result.exitCode).toBe(0); + expect(result.browserExitCode).toBe('0'); + expect(result.browserSignal).toBe('null'); + }); + it('should kill the browser on double SIGINT', async () => { + const result = await testSignal(child => { + process.kill(child.pid, 'SIGINT'); + process.kill(child.pid, 'SIGINT'); + }); + expect(result.exitCode).toBe(130); + // TODO: ideally, we would expect the SIGKILL on the browser from + // force kill, but that's racy with sending two signals. + }); + it('should kill the browser on SIGINT + SIGTERM', async () => { + const result = await testSignal(child => { + process.kill(child.pid, 'SIGINT'); + process.kill(child.pid, 'SIGTERM'); + }); + expect(result.exitCode).toBe(130); + // TODO: ideally, we would expect the SIGKILL on the browser from + // force kill, but that's racy with sending two signals. + }); + it('should kill the browser on SIGTERM + SIGINT', async () => { + const result = await testSignal(child => { + process.kill(child.pid, 'SIGTERM'); + process.kill(child.pid, 'SIGINT'); + }); + expect(result.exitCode).toBe(130); + // TODO: ideally, we would expect the SIGKILL on the browser from + // force kill, but that's racy with sending two signals. + }); + } }); }; diff --git a/test/fixtures/closeme.js b/test/fixtures/closeme.js index 7b527d09d9..46119b3902 100644 --- a/test/fixtures/closeme.js +++ b/test/fixtures/closeme.js @@ -1,5 +1,9 @@ (async() => { const [, , playwrightRoot, product, options] = process.argv; const browserApp = await require(playwrightRoot)[product.toLowerCase()].launchBrowserApp(JSON.parse(options)); - console.log(browserApp.wsEndpoint()); + browserApp.on('close', (exitCode, signal) => { + console.log(`browserClose:${exitCode}:${signal}:browserClose`); + }); + console.log(`browserPid:${browserApp.process().pid}:browserPid`); + console.log(`browserWS:${browserApp.wsEndpoint()}:browserWS`); })(); diff --git a/test/launcher.spec.js b/test/launcher.spec.js index ac56a330b3..80954229e8 100644 --- a/test/launcher.spec.js +++ b/test/launcher.spec.js @@ -18,7 +18,6 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const util = require('util'); -const {spawn, execSync} = require('child_process'); const utils = require('./utils'); const rmAsync = util.promisify(require('rimraf')); @@ -67,32 +66,6 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p expect(page.url()).toBe(server.EMPTY_PAGE); await browser.close(); }); - it('should close the browser when the node process closes', async({ server }) => { - const options = Object.assign({}, defaultBrowserOptions, { - // Disable DUMPIO to cleanly read stdout. - dumpio: false, - webSocket: true, - }); - const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), playwrightPath, product, JSON.stringify(options)]); - let wsEndPointCallback; - const wsEndPointPromise = new Promise(x => wsEndPointCallback = x); - let output = ''; - res.stdout.on('data', data => { - output += data; - if (output.indexOf('\n')) - wsEndPointCallback(output.substring(0, output.indexOf('\n'))); - }); - const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise }); - const promises = [ - new Promise(resolve => browser.once('disconnected', resolve)), - new Promise(resolve => res.on('exit', resolve)) - ]; - if (process.platform === 'win32') - execSync(`taskkill /pid ${res.pid} /T /F`); - else - process.kill(res.pid); - await Promise.all(promises); - }); it('should return child_process instance', async () => { const browserApp = await playwright.launchBrowserApp(defaultBrowserOptions); expect(browserApp.process().pid).toBeGreaterThan(0);