From 6db64985656b5a536b9aa8f1405356380dbc5d58 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 30 May 2023 13:54:04 -0700 Subject: [PATCH] chore: replace sigint handler per browser with a single one (#23317) Otherwise, multiple sigint handlers (one from each browser) would try to `process.exit(130)` each. --- .../src/utils/processLauncher.ts | 86 +++++++++++++------ tests/config/remoteServer.ts | 1 + tests/library/signals.spec.ts | 3 +- 3 files changed, 65 insertions(+), 25 deletions(-) diff --git a/packages/playwright-core/src/utils/processLauncher.ts b/packages/playwright-core/src/utils/processLauncher.ts index e043ebe854..496ea7304a 100644 --- a/packages/playwright-core/src/utils/processLauncher.ts +++ b/packages/playwright-core/src/utils/processLauncher.ts @@ -18,7 +18,6 @@ import * as childProcess from 'child_process'; import * as readline from 'readline'; import * as path from 'path'; -import { eventsHelper } from './eventsHelper'; import { isUnderTest } from './'; import { removeFolders } from './fileUtils'; @@ -51,16 +50,63 @@ type LaunchResult = { }; export const gracefullyCloseSet = new Set<() => Promise>(); +const killSet = new Set<() => void>(); export async function gracefullyCloseAll() { await Promise.all(Array.from(gracefullyCloseSet).map(gracefullyClose => gracefullyClose().catch(e => {}))); } -// We currently spawn a process per page when recording video in Chromium. -// This triggers "too many listeners" on the process object once you have more than 10 pages open. -const maxListeners = process.getMaxListeners(); -if (maxListeners !== 0) - process.setMaxListeners(Math.max(maxListeners || 0, 100)); +function exitHandler() { + for (const kill of killSet) + kill(); +} + +let sigintHandlerCalled = false; +function sigintHandler() { + const exitWithCode130 = () => { + // Give tests a chance to see that launched process did exit and dispatch any async calls. + if (isUnderTest()) + setTimeout(() => process.exit(130), 1000); + else + process.exit(130); + }; + + if (sigintHandlerCalled) { + // Resort to default handler from this point on, just in case we hang/stall. + process.off('SIGINT', sigintHandler); + + // Upon second Ctrl+C, immediately kill browsers and exit. + // This prevents hanging in the case where closing the browser takes a lot of time or is buggy. + for (const kill of killSet) + kill(); + exitWithCode130(); + } else { + sigintHandlerCalled = true; + gracefullyCloseAll().then(() => exitWithCode130()); + } +} + +function sigtermHandler() { + gracefullyCloseAll(); +} + +function sighupHandler() { + gracefullyCloseAll(); +} + +const installedHandlers = new Set<'exit' | 'SIGINT' | 'SIGTERM' | 'SIGHUP'>(); +const processHandlers = { + exit: exitHandler, + SIGINT: sigintHandler, + SIGTERM: sigtermHandler, + SIGHUP: sighupHandler, +}; +function addProcessHandlerIfNeeded(name: 'exit' | 'SIGINT' | 'SIGTERM' | 'SIGHUP') { + if (!installedHandlers.has(name)) { + installedHandlers.add(name); + process.on(name, processHandlers[name]); + } +} export async function launchProcess(options: LaunchProcessOptions): Promise { const stdio: ('ignore' | 'pipe')[] = options.stdio === 'pipe' ? ['ignore', 'pipe', 'pipe', 'pipe', 'pipe'] : ['pipe', 'pipe', 'pipe']; @@ -116,34 +162,25 @@ export async function launchProcess(options: LaunchProcessOptions): Promise { options.log(`[pid=${spawnedProcess.pid}] `); processClosed = true; - eventsHelper.removeEventListeners(listeners); gracefullyCloseSet.delete(gracefullyClose); + killSet.delete(killProcessAndCleanup); options.onExit(exitCode, signal); // Cleanup as process exits. cleanup().then(fulfillCleanup); }); - const listeners = [eventsHelper.addEventListener(process, 'exit', killProcessAndCleanup)]; - if (options.handleSIGINT) { - listeners.push(eventsHelper.addEventListener(process, 'SIGINT', () => { - gracefullyClose().then(() => { - // Give tests a chance to dispatch any async calls. - if (isUnderTest()) - setTimeout(() => process.exit(130), 0); - else - process.exit(130); - }); - })); - } + addProcessHandlerIfNeeded('exit'); + if (options.handleSIGINT) + addProcessHandlerIfNeeded('SIGINT'); if (options.handleSIGTERM) - listeners.push(eventsHelper.addEventListener(process, 'SIGTERM', gracefullyClose)); + addProcessHandlerIfNeeded('SIGTERM'); if (options.handleSIGHUP) - listeners.push(eventsHelper.addEventListener(process, 'SIGHUP', gracefullyClose)); + addProcessHandlerIfNeeded('SIGHUP'); gracefullyCloseSet.add(gracefullyClose); + killSet.add(killProcessAndCleanup); let gracefullyClosing = false; async function gracefullyClose(): 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. @@ -161,10 +198,11 @@ export async function launchProcess(options: LaunchProcessOptions): Promise`); } - // This method has to be sync to be used as 'exit' event handler. + // This method has to be sync to be used in the 'exit' event handler. function killProcess() { + gracefullyCloseSet.delete(gracefullyClose); + killSet.delete(killProcessAndCleanup); options.log(`[pid=${spawnedProcess.pid}] `); - eventsHelper.removeEventListeners(listeners); if (spawnedProcess.pid && !spawnedProcess.killed && !processClosed) { options.log(`[pid=${spawnedProcess.pid}] `); // Force kill the browser. diff --git a/tests/config/remoteServer.ts b/tests/config/remoteServer.ts index 2419c8bd50..2b81abba7c 100644 --- a/tests/config/remoteServer.ts +++ b/tests/config/remoteServer.ts @@ -103,6 +103,7 @@ export class RemoteServer implements PlaywrightServer { }; this._process = childProcess({ command: ['node', path.join(__dirname, 'remote-server-impl.js'), JSON.stringify(options)], + env: { ...process.env, PWTEST_UNDER_TEST: '1' }, }); let index = 0; diff --git a/tests/library/signals.spec.ts b/tests/library/signals.spec.ts index f4ccef8187..50b34c15e5 100644 --- a/tests/library/signals.spec.ts +++ b/tests/library/signals.spec.ts @@ -119,7 +119,8 @@ test.describe('signals', () => { expect(await remoteServer.childExitCode()).toBe(0); }); - test('should kill the browser on SIGTERM + SIGINT', async ({ startRemoteServer, server }) => { + test('should kill the browser on SIGTERM + SIGINT', async ({ startRemoteServer, server, isMac, browserName }) => { + test.fixme(isMac && browserName === 'webkit' && parseInt(os.release(), 10) >= 22, 'https://github.com/microsoft/playwright/issues/22226'); const remoteServer = await startRemoteServer('launchServer', { stallOnClose: true, url: server.EMPTY_PAGE }); process.kill(remoteServer.child().pid, 'SIGTERM'); await remoteServer.out('stalled');