From 91e1a25f34b444914369bc1f89bed95a6df194d2 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 17 Jul 2020 16:14:23 -0700 Subject: [PATCH] feat(rpc): gracefully close browsers in server process on disconnect (#3005) --- src/rpc/server.ts | 9 ++++++- src/server/processLauncher.ts | 9 +++++++ test/chromium/launcher.jest.js | 2 +- test/environments.js | 22 ++++++++-------- test/jest/fixtures.js | 41 +++++++++++++++--------------- test/jest/playwrightEnvironment.js | 3 ++- test/launcher.jest.js | 2 +- 7 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/rpc/server.ts b/src/rpc/server.ts index ee61e5191a7e1..a1480e11964cf 100644 --- a/src/rpc/server.ts +++ b/src/rpc/server.ts @@ -20,12 +20,19 @@ import { Playwright } from '../server/playwright'; import { PlaywrightDispatcher } from './server/playwrightDispatcher'; import { Electron } from '../server/electron'; import { setUseApiName } from '../progress'; +import { gracefullyCloseAll } from '../server/processLauncher'; setUseApiName(false); const dispatcherConnection = new DispatcherConnection(); const transport = new Transport(process.stdout, process.stdin); -transport.onclose = () => process.exit(0); +transport.onclose = async () => { + // Force exit after 30 seconds. + setTimeout(() => process.exit(0), 30000); + // Meanwhile, try to gracefully close all browsers. + await gracefullyCloseAll(); + process.exit(0); +}; transport.onmessage = message => dispatcherConnection.dispatch(JSON.parse(message)); dispatcherConnection.onmessage = message => transport.send(JSON.stringify(message)); diff --git a/src/server/processLauncher.ts b/src/server/processLauncher.ts index 08d2da0aa30c8..60b398fc8d94a 100644 --- a/src/server/processLauncher.ts +++ b/src/server/processLauncher.ts @@ -49,6 +49,12 @@ type LaunchResult = { kill: () => Promise, }; +const gracefullyCloseSet = new Set<() => Promise>(); + +export async function gracefullyCloseAll() { + await Promise.all(Array.from(gracefullyCloseSet).map(gracefullyClose => gracefullyClose().catch(e => {}))); +} + export async function launchProcess(options: LaunchProcessOptions): Promise { const cleanup = () => helper.removeFolders(options.tempDirectories); @@ -97,6 +103,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise`); processClosed = true; helper.removeEventListeners(listeners); + gracefullyCloseSet.delete(gracefullyClose); options.onExit(exitCode, signal); fulfillClose(); // Cleanup as process exits. @@ -113,9 +120,11 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { + gracefullyCloseSet.delete(gracefullyClose); // 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, for example user sends SIGINT second time. diff --git a/test/chromium/launcher.jest.js b/test/chromium/launcher.jest.js index 8a2c7ea44741e..c47720790d1e1 100644 --- a/test/chromium/launcher.jest.js +++ b/test/chromium/launcher.jest.js @@ -32,7 +32,7 @@ describe.skip(!CHROMIUM)('launcher', function() { const browser = await browserType.launchServer(options); await browser.close(); }); - it.skip(USES_HOOKS).fail(CHROMIUM && WIN)('should open devtools when "devtools: true" option is given', async({browserType, defaultBrowserOptions}) => { + it.fail(USES_HOOKS || WIN)('should open devtools when "devtools: true" option is given', async({browserType, defaultBrowserOptions}) => { let devtoolsCallback; const devtoolsPromise = new Promise(f => devtoolsCallback = f); const __testHookForDevTools = devtools => devtools.__testHookOnBinding = parsed => { diff --git a/test/environments.js b/test/environments.js index 0478b8c971569..369b1cdbf4476 100644 --- a/test/environments.js +++ b/test/environments.js @@ -161,7 +161,7 @@ class PlaywrightEnvironment { constructor(playwright) { this._playwright = playwright; this.spawnedProcess = undefined; - this.expectExit = false; + this.onExit = undefined; } name() { return 'Playwright'; }; @@ -173,13 +173,13 @@ class PlaywrightEnvironment { if (process.env.PWCHANNEL === 'wire') { this.spawnedProcess = childProcess.fork(path.join(__dirname, '..', 'lib', 'rpc', 'server'), [], { stdio: 'pipe', - detached: process.platform !== 'win32', - }); - this.spawnedProcess.once('exit', (exitCode, signal) => { - this.spawnedProcess = undefined; - if (!this.expectExit) - throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`); + detached: true, }); + this.spawnedProcess.unref(); + this.onExit = (exitCode, signal) => { + throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`); + }; + this.spawnedProcess.once('exit', this.onExit); const transport = new Transport(this.spawnedProcess.stdin, this.spawnedProcess.stdout); connection.onmessage = message => transport.send(JSON.stringify(message)); transport.onmessage = message => connection.dispatch(JSON.parse(message)); @@ -205,10 +205,10 @@ class PlaywrightEnvironment { async afterAll(state) { if (this.spawnedProcess) { - const exited = new Promise(f => this.spawnedProcess.once('exit', f)); - this.expectExit = true; - this.spawnedProcess.kill(); - await exited; + this.spawnedProcess.removeListener('exit', this.onExit); + this.spawnedProcess.stdin.destroy(); + this.spawnedProcess.stdout.destroy(); + this.spawnedProcess.stderr.destroy(); } delete state.playwright; } diff --git a/test/jest/fixtures.js b/test/jest/fixtures.js index a316527938580..bc2d40ff3f4e5 100644 --- a/test/jest/fixtures.js +++ b/test/jest/fixtures.js @@ -40,7 +40,7 @@ module.exports = function registerFixtures(global) { server.PREFIX = `http://localhost:${port}`; server.CROSS_PROCESS_PREFIX = `http://127.0.0.1:${port}`; server.EMPTY_PAGE = `http://localhost:${port}/empty.html`; - + const httpsPort = port + 1; const httpsServer = await TestServer.createHTTPS(assetsPath, httpsPort); httpsServer.enableHTTPCache(cachedPath); @@ -48,13 +48,13 @@ module.exports = function registerFixtures(global) { httpsServer.PREFIX = `https://localhost:${httpsPort}`; httpsServer.CROSS_PROCESS_PREFIX = `https://127.0.0.1:${httpsPort}`; httpsServer.EMPTY_PAGE = `https://localhost:${httpsPort}/empty.html`; - + await test({server, httpsServer}); await Promise.all([ server.stop(), httpsServer.stop(), - ]); + ]); }); global.registerWorkerFixture('defaultBrowserOptions', async({}, test) => { @@ -72,17 +72,17 @@ module.exports = function registerFixtures(global) { const connection = new Connection(); let toImpl; let spawnedProcess; - let expectExit; + let onExit; if (process.env.PWCHANNEL === 'wire') { spawnedProcess = childProcess.fork(path.join(__dirname, '..', '..', 'lib', 'rpc', 'server'), [], { stdio: 'pipe', - detached: process.platform !== 'win32', - }); - spawnedProcess.once('exit', (exitCode, signal) => { - spawnedProcess = undefined; - if (!expectExit) - throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`); + detached: true, }); + spawnedProcess.unref(); + onExit = (exitCode, signal) => { + throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`); + }; + spawnedProcess.on('exit', onExit); const transport = new Transport(spawnedProcess.stdin, spawnedProcess.stdout); connection.onmessage = message => transport.send(JSON.stringify(message)); transport.onmessage = message => connection.dispatch(JSON.parse(message)); @@ -103,17 +103,16 @@ module.exports = function registerFixtures(global) { const playwrightObject = await connection.waitForObjectWithKnownName('playwright'); playwrightObject.toImpl = toImpl; await test(playwrightObject); - if (spawnedProcess) { - const exited = new Promise(f => spawnedProcess.once('exit', f)); - expectExit = true; - spawnedProcess.kill(); - await exited; + spawnedProcess.removeListener('exit', onExit); + spawnedProcess.stdin.destroy(); + spawnedProcess.stdout.destroy(); + spawnedProcess.stderr.destroy(); } - return; + } else { + playwright.toImpl = x => x; + await test(playwright); } - playwright.toImpl = x => x; - await test(playwright); }); global.registerFixture('toImpl', async ({playwright}, test) => { @@ -123,7 +122,7 @@ module.exports = function registerFixtures(global) { global.registerWorkerFixture('browserType', async ({playwright}, test) => { await test(playwright[process.env.BROWSER || 'chromium']); }); - + global.registerWorkerFixture('browser', async ({browserType, defaultBrowserOptions}, test) => { const browser = await browserType.launch(defaultBrowserOptions); try { @@ -136,7 +135,7 @@ module.exports = function registerFixtures(global) { await browser.close(); } }); - + global.registerFixture('context', async ({browser}, test) => { const context = await browser.newContext(); try { @@ -145,7 +144,7 @@ module.exports = function registerFixtures(global) { await context.close(); } }); - + global.registerFixture('page', async ({context}, test) => { const page = await context.newPage(); await test(page); diff --git a/test/jest/playwrightEnvironment.js b/test/jest/playwrightEnvironment.js index fe58205c844a7..a59e092861bd6 100644 --- a/test/jest/playwrightEnvironment.js +++ b/test/jest/playwrightEnvironment.js @@ -81,6 +81,7 @@ class PlaywrightEnvironment extends NodeEnvironment { this.global.describe.fail = this.global.describe.skip; const itSkip = this.global.it.skip; + itSkip.slow = () => itSkip; this.global.it.skip = (...args) => { if (args.length = 1) return args[0] ? itSkip : this.global.it; @@ -199,7 +200,7 @@ class FixturePool { await this.setupFixture(name); const params = {}; for (const n of names) - params[n] = this.instances.get(n).value; + params[n] = this.instances.get(n).value; return fn(params); } } diff --git a/test/launcher.jest.js b/test/launcher.jest.js index b942b8a06aee8..0a866fd60b224 100644 --- a/test/launcher.jest.js +++ b/test/launcher.jest.js @@ -301,7 +301,7 @@ describe('browserType.connect', function() { await browserServer._checkLeaks(); await browserServer.close(); }); - it.skip(USES_HOOKS).slow().fail(CHROMIUM && WIN)('should handle exceptions during connect', async({browserType, defaultBrowserOptions, server}) => { + it.fail(USES_HOOKS || (CHROMIUM && WIN)).slow()('should handle exceptions during connect', async({browserType, defaultBrowserOptions, server}) => { const browserServer = await browserType.launchServer(defaultBrowserOptions); const __testHookBeforeCreateBrowser = () => { throw new Error('Dummy') }; const error = await browserType.connect({ wsEndpoint: browserServer.wsEndpoint(), __testHookBeforeCreateBrowser }).catch(e => e);