From b4c412eb1fe11ed91454216b4477ddafee20a3d1 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 24 Jul 2023 08:29:29 -0700 Subject: [PATCH] chore: replace process.exit with graceful closure (#24242) Everywhere we call `process.exit()`, we might actually need to gracefully close all browsers. --- packages/.eslintrc.js | 7 ++- packages/playwright-core/src/cli/cli.ts | 6 +-- packages/playwright-core/src/cli/driver.ts | 15 ++---- packages/playwright-core/src/cli/program.ts | 49 +++++++++---------- .../src/server/debugController.ts | 15 ++---- .../server/registry/oopDownloadBrowserMain.ts | 1 + .../src/server/trace/viewer/traceViewer.ts | 17 ++----- .../src/utils/processLauncher.ts | 18 ++++++- packages/playwright-test/src/cli.ts | 19 ++++--- .../playwright-test/src/common/process.ts | 2 + .../src/plugins/webServerPlugin.ts | 4 +- .../playwright-test/src/reporters/html.ts | 4 +- .../playwright-test/src/worker/workerMain.ts | 5 +- 13 files changed, 78 insertions(+), 84 deletions(-) diff --git a/packages/.eslintrc.js b/packages/.eslintrc.js index bc96726981..54c58ce323 100644 --- a/packages/.eslintrc.js +++ b/packages/.eslintrc.js @@ -11,6 +11,11 @@ module.exports = { */ "rules": { "no-console": 2, - "no-debugger": 2 + "no-debugger": 2, + "no-restricted-properties": [2, { + "object": "process", + "property": "exit", + "message": "Please use gracefullyProcessExitDoNotHang function to exit the process.", + }], } }; diff --git a/packages/playwright-core/src/cli/cli.ts b/packages/playwright-core/src/cli/cli.ts index 600f544ac9..ac3cacff9d 100755 --- a/packages/playwright-core/src/cli/cli.ts +++ b/packages/playwright-core/src/cli/cli.ts @@ -18,7 +18,7 @@ /* eslint-disable no-console */ -import { getPackageManager } from '../utils'; +import { getPackageManager, gracefullyProcessExitDoNotHang } from '../utils'; import program from './program'; function printPlaywrightTestError(command: string) { @@ -53,7 +53,7 @@ function printPlaywrightTestError(command: string) { command.description('Run tests with Playwright Test. Available in @playwright/test package.'); command.action(async () => { printPlaywrightTestError('test'); - process.exit(1); + gracefullyProcessExitDoNotHang(1); }); } @@ -62,7 +62,7 @@ function printPlaywrightTestError(command: string) { command.description('Show Playwright Test HTML report. Available in @playwright/test package.'); command.action(async () => { printPlaywrightTestError('show-report'); - process.exit(1); + gracefullyProcessExitDoNotHang(1); }); } diff --git a/packages/playwright-core/src/cli/driver.ts b/packages/playwright-core/src/cli/driver.ts index 547ed576e8..3bb22c3cbe 100644 --- a/packages/playwright-core/src/cli/driver.ts +++ b/packages/playwright-core/src/cli/driver.ts @@ -23,7 +23,7 @@ import type { LaunchServerOptions } from '../client/types'; import { createPlaywright, DispatcherConnection, RootDispatcher, PlaywrightDispatcher } from '../server'; import { PipeTransport } from '../protocol/transport'; import { PlaywrightServer } from '../remote/playwrightServer'; -import { gracefullyCloseAll } from '../utils/processLauncher'; +import { gracefullyProcessExitDoNotHang } from '../utils/processLauncher'; export function printApiJson() { // Note: this file is generated by build-playwright-driver.sh @@ -42,7 +42,7 @@ export function runDriver() { transport.onclose = () => { // Drop any messages during shutdown on the floor. dispatcherConnection.onmessage = () => {}; - selfDestruct(); + gracefullyProcessExitDoNotHang(0); }; // Ignore the SIGINT signal in the driver process so the parent can gracefully close the connection. // We still will destruct everything (close browsers and exit) when the transport pipe closes. @@ -71,7 +71,7 @@ export async function runServer(options: RunServerOptions) { const wsEndpoint = await server.listen(port); process.on('exit', () => server.close().catch(console.error)); console.log('Listening on ' + wsEndpoint); // eslint-disable-line no-console - process.stdin.on('close', () => selfDestruct()); + process.stdin.on('close', () => gracefullyProcessExitDoNotHang(0)); } export async function launchBrowserServer(browserName: string, configFile?: string) { @@ -82,12 +82,3 @@ export async function launchBrowserServer(browserName: string, configFile?: stri const server = await browserType.launchServer(options); console.log(server.wsEndpoint()); } - -function selfDestruct() { - // Force exit after 30 seconds. - setTimeout(() => process.exit(0), 30000); - // Meanwhile, try to gracefully close all browsers. - gracefullyCloseAll().then(() => { - process.exit(0); - }); -} diff --git a/packages/playwright-core/src/cli/program.ts b/packages/playwright-core/src/cli/program.ts index 80ade0a375..a85ed67c11 100644 --- a/packages/playwright-core/src/cli/program.ts +++ b/packages/playwright-core/src/cli/program.ts @@ -31,7 +31,7 @@ import type { Page } from '../client/page'; import type { BrowserType } from '../client/browserType'; import type { BrowserContextOptions, LaunchOptions } from '../client/types'; import { spawn } from 'child_process'; -import { wrapInASCIIBox, isLikelyNpxGlobal, assert } from '../utils'; +import { wrapInASCIIBox, isLikelyNpxGlobal, assert, gracefullyProcessExitDoNotHang } from '../utils'; import type { Executable } from '../server'; import { registry, writeDockerVersion } from '../server'; @@ -104,10 +104,8 @@ function checkBrowsersToInstall(args: string[]): Executable[] { else executables.push(executable); } - if (faultyArguments.length) { - console.log(`Invalid installation targets: ${faultyArguments.map(name => `'${name}'`).join(', ')}. Expecting one of: ${suggestedBrowsersToInstall()}`); - process.exit(1); - } + if (faultyArguments.length) + throw new Error(`Invalid installation targets: ${faultyArguments.map(name => `'${name}'`).join(', ')}. Expecting one of: ${suggestedBrowsersToInstall()}`); return executables; } @@ -163,7 +161,7 @@ program } } catch (e) { console.log(`Failed to install browsers\n${e}`); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } }).addHelpText('afterAll', ` @@ -199,7 +197,7 @@ program await registry.installDeps(checkBrowsersToInstall(args), !!options.dryRun); } catch (e) { console.log(`Failed to install browser dependencies\n${e}`); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } }).addHelpText('afterAll', ` Examples: @@ -308,7 +306,7 @@ program openTraceInBrowser(traces, openOptions).catch(logErrorAndExit); } else { openTraceViewerApp(traces, options.browser, openOptions).then(page => { - page.on('close', () => process.exit(0)); + page.on('close', () => gracefullyProcessExitDoNotHang(0)); }).catch(logErrorAndExit); } }).addHelpText('afterAll', ` @@ -406,7 +404,7 @@ async function launchContext(options: Options, headless: boolean, executablePath const hasCrashLine = logs.some(line => line.includes('process did exit:') && !line.includes('process did exit: exitCode=0, signal=null')); if (hasCrashLine) { process.stderr.write('Detected browser crash.\n'); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } }); } @@ -417,8 +415,7 @@ async function launchContext(options: Options, headless: boolean, executablePath const [width, height] = options.viewportSize.split(',').map(n => parseInt(n, 10)); contextOptions.viewport = { width, height }; } catch (e) { - console.log('Invalid window size format: use "width, height", for example --window-size=800,600'); - process.exit(0); + throw new Error('Invalid viewport size format: use "width, height", for example --viewport-size=800,600'); } } @@ -432,8 +429,7 @@ async function launchContext(options: Options, headless: boolean, executablePath longitude }; } catch (e) { - console.log('Invalid geolocation format: user lat, long, for example --geolocation="37.819722,-122.478611"'); - process.exit(0); + throw new Error('Invalid geolocation format, should be "lat,long". For example --geolocation="37.819722,-122.478611"'); } contextOptions.permissions = ['geolocation']; } @@ -507,7 +503,7 @@ async function launchContext(options: Options, headless: boolean, executablePath }); process.on('SIGINT', async () => { await closeBrowser(); - process.exit(130); + gracefullyProcessExitDoNotHang(130); }); const timeout = options.timeout ? parseInt(options.timeout, 10) : 0; @@ -596,10 +592,8 @@ async function screenshot(options: Options, captureOptions: CaptureOptions, url: } async function pdf(options: Options, captureOptions: CaptureOptions, url: string, path: string) { - if (options.browser !== 'chromium') { - console.error('PDF creation is only working with Chromium'); - process.exit(1); - } + if (options.browser !== 'chromium') + throw new Error('PDF creation is only working with Chromium'); const { context } = await launchContext({ ...options, browser: 'chromium' }, true); console.log('Navigating to ' + url); const page = await openPage(context, url); @@ -632,20 +626,21 @@ function lookupBrowserType(options: Options): BrowserType { function validateOptions(options: Options) { if (options.device && !(options.device in playwright.devices)) { - console.log(`Device descriptor not found: '${options.device}', available devices are:`); + const lines = [`Device descriptor not found: '${options.device}', available devices are:`]; for (const name in playwright.devices) - console.log(` "${name}"`); - process.exit(0); - } - if (options.colorScheme && !['light', 'dark'].includes(options.colorScheme)) { - console.log('Invalid color scheme, should be one of "light", "dark"'); - process.exit(0); + lines.push(` "${name}"`); + throw new Error(lines.join('\n')); } + if (options.colorScheme && !['light', 'dark'].includes(options.colorScheme)) + throw new Error('Invalid color scheme, should be one of "light", "dark"'); } function logErrorAndExit(e: Error) { - console.error(e); - process.exit(1); + if (process.env.PWDEBUGIMPL) + console.error(e); + else + console.error(e.name + ': ' + e.message); + gracefullyProcessExitDoNotHang(1); } function codegenId(): string { diff --git a/packages/playwright-core/src/server/debugController.ts b/packages/playwright-core/src/server/debugController.ts index dc274bf3b7..301b227c58 100644 --- a/packages/playwright-core/src/server/debugController.ts +++ b/packages/playwright-core/src/server/debugController.ts @@ -15,7 +15,7 @@ */ import type { Mode, Source } from '@recorder/recorderTypes'; -import { gracefullyCloseAll } from '../utils/processLauncher'; +import { gracefullyProcessExitDoNotHang } from '../utils/processLauncher'; import type { Browser } from './browser'; import type { BrowserContext } from './browserContext'; import { createInstrumentation, SdkObject, serverSideCallMetadata } from './instrumentation'; @@ -138,7 +138,7 @@ export class DebugController extends SdkObject { return; const heartBeat = () => { if (!this._playwright.allPages().length) - selfDestruct(); + gracefullyProcessExitDoNotHang(0); else this._autoCloseTimer = setTimeout(heartBeat, 5000); }; @@ -168,7 +168,7 @@ export class DebugController extends SdkObject { } async kill() { - selfDestruct(); + gracefullyProcessExitDoNotHang(0); } async closeAllBrowsers() { @@ -218,15 +218,6 @@ export class DebugController extends SdkObject { } } -function selfDestruct() { - // Force exit after 30 seconds. - setTimeout(() => process.exit(0), 30000); - // Meanwhile, try to gracefully close all browsers. - gracefullyCloseAll().then(() => { - process.exit(0); - }); -} - class InspectingRecorderApp extends EmptyRecorderApp { private _debugController: DebugController; diff --git a/packages/playwright-core/src/server/registry/oopDownloadBrowserMain.ts b/packages/playwright-core/src/server/registry/oopDownloadBrowserMain.ts index 3ff5db3661..b79e3717f5 100644 --- a/packages/playwright-core/src/server/registry/oopDownloadBrowserMain.ts +++ b/packages/playwright-core/src/server/registry/oopDownloadBrowserMain.ts @@ -166,5 +166,6 @@ async function main() { main().catch(error => { // eslint-disable-next-line no-console console.error(error); + // eslint-disable-next-line no-restricted-properties process.exit(1); }); diff --git a/packages/playwright-core/src/server/trace/viewer/traceViewer.ts b/packages/playwright-core/src/server/trace/viewer/traceViewer.ts index 52e99e7a10..3b5cf78dab 100644 --- a/packages/playwright-core/src/server/trace/viewer/traceViewer.ts +++ b/packages/playwright-core/src/server/trace/viewer/traceViewer.ts @@ -18,7 +18,7 @@ import path from 'path'; import fs from 'fs'; import { HttpServer } from '../../../utils/httpServer'; import { findChromiumChannel } from '../../registry'; -import { createGuid, gracefullyCloseAll, isUnderTest } from '../../../utils'; +import { createGuid, gracefullyProcessExitDoNotHang, isUnderTest } from '../../../utils'; import { installAppIcon, syncLocalStorageWithSettings } from '../../chromium/crApp'; import { serverSideCallMetadata } from '../../instrumentation'; import { createPlaywright } from '../../playwright'; @@ -54,7 +54,7 @@ async function startTraceViewerServer(traceUrls: string[], options?: OpenTraceVi if (!traceUrl.startsWith('http://') && !traceUrl.startsWith('https://') && !fs.existsSync(traceFile) && !fs.existsSync(traceFile + '.trace')) { // eslint-disable-next-line no-console console.error(`Trace file ${traceUrl} does not exist!`); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } } @@ -192,7 +192,7 @@ class StdinServer implements Transport { else this._loadTrace(url); }); - process.stdin.on('close', () => this._selfDestruct()); + process.stdin.on('close', () => gracefullyProcessExitDoNotHang(0)); } async dispatch(method: string, params: any) { @@ -203,7 +203,7 @@ class StdinServer implements Transport { } onclose() { - this._selfDestruct(); + gracefullyProcessExitDoNotHang(0); } sendEvent?: (method: string, params: any) => void; @@ -221,15 +221,6 @@ class StdinServer implements Transport { this._pollLoadTrace(url); }, 500); } - - private _selfDestruct() { - // Force exit after 30 seconds. - setTimeout(() => process.exit(0), 30000); - // Meanwhile, try to gracefully close all browsers. - gracefullyCloseAll().then(() => { - process.exit(0); - }); - } } function traceDescriptor(traceName: string) { diff --git a/packages/playwright-core/src/utils/processLauncher.ts b/packages/playwright-core/src/utils/processLauncher.ts index 496ea7304a..184241f8f0 100644 --- a/packages/playwright-core/src/utils/processLauncher.ts +++ b/packages/playwright-core/src/utils/processLauncher.ts @@ -56,6 +56,17 @@ export async function gracefullyCloseAll() { await Promise.all(Array.from(gracefullyCloseSet).map(gracefullyClose => gracefullyClose().catch(e => {}))); } +export function gracefullyProcessExitDoNotHang(code: number) { + // Force exit after 30 seconds. + // eslint-disable-next-line no-restricted-properties + setTimeout(() => process.exit(code), 30000); + // Meanwhile, try to gracefully close all browsers. + gracefullyCloseAll().then(() => { + // eslint-disable-next-line no-restricted-properties + process.exit(code); + }); +} + function exitHandler() { for (const kill of killSet) kill(); @@ -65,10 +76,13 @@ 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()) + if (isUnderTest()) { + // eslint-disable-next-line no-restricted-properties setTimeout(() => process.exit(130), 1000); - else + } else { + // eslint-disable-next-line no-restricted-properties process.exit(130); + } }; if (sigintHandlerCalled) { diff --git a/packages/playwright-test/src/cli.ts b/packages/playwright-test/src/cli.ts index 6345dd0f5e..f1ee0bc64a 100644 --- a/packages/playwright-test/src/cli.ts +++ b/packages/playwright-test/src/cli.ts @@ -20,7 +20,7 @@ import type { Command } from 'playwright-core/lib/utilsBundle'; import fs from 'fs'; import path from 'path'; import { Runner } from './runner/runner'; -import { stopProfiling, startProfiling } from 'playwright-core/lib/utils'; +import { stopProfiling, startProfiling, gracefullyProcessExitDoNotHang } from 'playwright-core/lib/utils'; import { experimentalLoaderOption, fileIsModule, serializeError } from './util'; import { showHTMLReport } from './reporters/html'; import { createMergedReport } from './reporters/merge'; @@ -44,7 +44,7 @@ function addTestCommand(program: Command) { await runTests(args, opts); } catch (e) { console.error(e); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } }); command.addHelpText('afterAll', ` @@ -68,7 +68,7 @@ function addListFilesCommand(program: Command) { await listTestFiles(opts); } catch (e) { console.error(e); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } }); } @@ -96,7 +96,7 @@ function addMergeReportsCommand(program: Command) { await mergeReports(dir, options); } catch (e) { console.error(e); - process.exit(1); + gracefullyProcessExitDoNotHang(1); } }); command.option('-c, --config ', `Configuration file. Can be used to specify additional configuration for the output report.`); @@ -143,9 +143,8 @@ async function runTests(args: string[], opts: { [key: string]: any }) { else status = await runner.runAllTests(); await stopProfiling('runner'); - if (status === 'interrupted') - process.exit(130); - process.exit(status === 'passed' ? 0 : 1); + const exitCode = status === 'interrupted' ? 130 : (status === 'passed' ? 0 : 1); + gracefullyProcessExitDoNotHang(exitCode); } async function listTestFiles(opts: { [key: string]: any }) { @@ -164,13 +163,13 @@ async function listTestFiles(opts: { [key: string]: any }) { const runner = new Runner(config); const report = await runner.listTestFiles(opts.project); stdoutWrite(JSON.stringify(report), () => { - process.exit(0); + gracefullyProcessExitDoNotHang(0); }); } catch (e) { const error: TestError = serializeError(e); error.location = prepareErrorStack(e.stack).location; stdoutWrite(JSON.stringify({ error }), () => { - process.exit(0); + gracefullyProcessExitDoNotHang(0); }); } } @@ -288,7 +287,7 @@ function restartWithExperimentalTsEsm(configFile: string | null): boolean { innerProcess.on('close', (code: number | null) => { if (code !== 0 && code !== null) - process.exit(code); + gracefullyProcessExitDoNotHang(code); }); return true; } diff --git a/packages/playwright-test/src/common/process.ts b/packages/playwright-test/src/common/process.ts index 73b70d0171..8847c7f1c1 100644 --- a/packages/playwright-test/src/common/process.ts +++ b/packages/playwright-test/src/common/process.ts @@ -91,10 +91,12 @@ async function gracefullyCloseAndExit() { return; closed = true; // Force exit after 30 seconds. + // eslint-disable-next-line no-restricted-properties setTimeout(() => process.exit(0), 30000); // Meanwhile, try to gracefully shutdown. await processRunner.gracefullyClose().catch(() => {}); await stopProfiling(processName).catch(() => {}); + // eslint-disable-next-line no-restricted-properties process.exit(0); } diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts index 43c2bf7b2c..5443ff9736 100644 --- a/packages/playwright-test/src/plugins/webServerPlugin.ts +++ b/packages/playwright-test/src/plugins/webServerPlugin.ts @@ -99,7 +99,9 @@ export class WebServerPlugin implements TestRunnerPlugin { cwd: this._options.cwd, stdio: 'stdin', shell: true, - attemptToGracefullyClose: async () => {}, + // Reject to indicate that we cannot close the web server gracefully + // and should fallback to non-graceful shutdown. + attemptToGracefullyClose: () => Promise.reject(), log: () => {}, onExit: code => processExitedReject(new Error(code ? `Process from config.webServer was not able to start. Exit code: ${code}` : 'Process from config.webServer exited early.')), tempDirectories: [], diff --git a/packages/playwright-test/src/reporters/html.ts b/packages/playwright-test/src/reporters/html.ts index 38a19d343e..07705543fc 100644 --- a/packages/playwright-test/src/reporters/html.ts +++ b/packages/playwright-test/src/reporters/html.ts @@ -20,7 +20,7 @@ import path from 'path'; import type { TransformCallback } from 'stream'; import { Transform } from 'stream'; import type { FullConfig, Suite } from '../../types/testReporter'; -import { HttpServer, assert, calculateSha1, copyFileAndMakeWritable, removeFolders } from 'playwright-core/lib/utils'; +import { HttpServer, assert, calculateSha1, copyFileAndMakeWritable, gracefullyProcessExitDoNotHang, removeFolders } from 'playwright-core/lib/utils'; import type { JsonAttachment, JsonReport, JsonSuite, JsonTestCase, JsonTestResult, JsonTestStep } from './raw'; import RawReporter from './raw'; import { stripAnsiEscapes } from './base'; @@ -161,7 +161,7 @@ export async function showHTMLReport(reportFolder: string | undefined, host: str assert(fs.statSync(folder).isDirectory()); } catch (e) { console.log(colors.red(`No report found at "${folder}"`)); - process.exit(1); + gracefullyProcessExitDoNotHang(1); return; } const server = startHtmlReportServer(folder); diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts index 630d778147..d44a2ae12e 100644 --- a/packages/playwright-test/src/worker/workerMain.ts +++ b/packages/playwright-test/src/worker/workerMain.ts @@ -23,7 +23,7 @@ import { ConfigLoader } from '../common/configLoader'; import type { Suite, TestCase } from '../common/test'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; import { FixtureRunner } from './fixtureRunner'; -import { ManualPromise, captureLibraryStackTrace } from 'playwright-core/lib/utils'; +import { ManualPromise, captureLibraryStackTrace, gracefullyCloseAll } from 'playwright-core/lib/utils'; import { TestInfoImpl } from './testInfo'; import { TimeoutManager, type TimeSlot } from './timeoutManager'; import { ProcessRunner } from '../common/process'; @@ -109,6 +109,9 @@ export class WorkerMain extends ProcessRunner { // We have to load the project to get the right deadline below. await this._loadIfNeeded(); await this._teardownScopes(); + // Close any other browsers launched in this process. This includes anything launched + // manually in the test/hooks and internal browsers like Playwright Inspector. + await gracefullyCloseAll(); } catch (e) { this._fatalErrors.push(serializeError(e)); }