From c63a0b536d1f0119794a909e4f9c420c8506b4d5 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Fri, 21 Oct 2022 08:55:06 -0700 Subject: [PATCH] feat: send SIGTERM to webserver before SIGKILL'ing it. (#18220) We now will send `SIGTERM` to the webserver and wait for the `timeout` before sending `SIGKILL` to it. Fixes #18209 --- docs/src/test-api/class-testconfig.md | 2 +- .../src/utils/processLauncher.ts | 30 ++++++++++++++++--- .../src/plugins/webServerPlugin.ts | 12 ++++---- packages/playwright-test/types/test.d.ts | 3 +- .../assets/simple-server-ignores-sigterm.js | 13 ++++++++ tests/playwright-test/web-server.spec.ts | 25 ++++++++++++++++ 6 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 tests/playwright-test/assets/simple-server-ignores-sigterm.js diff --git a/docs/src/test-api/class-testconfig.md b/docs/src/test-api/class-testconfig.md index 6e98c8631f..11806e4e6c 100644 --- a/docs/src/test-api/class-testconfig.md +++ b/docs/src/test-api/class-testconfig.md @@ -656,7 +656,7 @@ export default config; - `port` ?<[int]> The port that your http server is expected to appear on. It does wait until it accepts connections. Exactly one of `port` or `url` is required. - `url` ?<[string]> The url on your http server that is expected to return a 2xx, 3xx, 400, 401, 402, or 403 status code when the server is ready to accept connections. Exactly one of `port` or `url` is required. - `ignoreHTTPSErrors` ?<[boolean]> Whether to ignore HTTPS errors when fetching the `url`. Defaults to `false`. - - `timeout` ?<[int]> How long to wait for the process to start up and be available in milliseconds. Defaults to 60000. + - `timeout` ?<[int]> How long to wait for the process to start up and be available in milliseconds. The same timeout is also used to terminate the process. Defaults to 60000. - `reuseExistingServer` ?<[boolean]> If true, it will re-use an existing server on the `port` or `url` when available. If no server is running on that `port` or `url`, it will run the command to start a new server. If `false`, it will throw if an existing process is listening on the `port` or `url`. This should be commonly set to `!process.env.CI` to allow the local dev server when running tests locally. - `cwd` ?<[string]> Current working directory of the spawned process, defaults to the directory of the configuration file. - `env` ?<[Object]<[string], [string]>> Environment variables to set for the command, `process.env` by default. diff --git a/packages/playwright-core/src/utils/processLauncher.ts b/packages/playwright-core/src/utils/processLauncher.ts index e043ebe854..b83e440f72 100644 --- a/packages/playwright-core/src/utils/processLauncher.ts +++ b/packages/playwright-core/src/utils/processLauncher.ts @@ -47,7 +47,7 @@ export type LaunchProcessOptions = { type LaunchResult = { launchedProcess: childProcess.ChildProcess, gracefullyClose: () => Promise, - kill: () => Promise, + kill: (sendSigtermBeforeSigkillTimeout?: number) => Promise, }; export const gracefullyCloseSet = new Set<() => Promise>(); @@ -188,6 +188,21 @@ export async function launchProcess(options: LaunchProcessOptions): Promise`); + try { + // Send SIGTERM to process tree. + process.kill(-spawnedProcess.pid, 'SIGTERM'); + } catch (e) { + // The process might have already stopped. + options.log(`[pid=${spawnedProcess.pid}] exception while trying to SIGTERM process: ${e}`); + } + } else { + options.log(`[pid=${spawnedProcess.pid}] `); + } + } + function killProcessAndCleanup() { killProcess(); options.log(`[pid=${spawnedProcess.pid || 'N/A'}] starting temporary directories cleanup`); @@ -202,9 +217,16 @@ export async function launchProcess(options: LaunchProcessOptions): Promise Promise; - private _killProcess?: () => Promise; + private _killProcess?: (presendSigtermBeforeSigkillTimeout?: number) => Promise; private _processExitedPromise!: Promise; private _options: WebServerPluginOptions; private _checkPortOnly: boolean; private _reporter?: Reporter; + private _launchTerminateTimeout: number; name = 'playwright:webserver'; constructor(options: WebServerPluginOptions, checkPortOnly: boolean) { this._options = options; + this._launchTerminateTimeout = this._options.timeout || 60 * 1000; this._checkPortOnly = checkPortOnly; } @@ -72,7 +74,8 @@ export class WebServerPlugin implements TestRunnerPlugin { } public async teardown() { - await this._killProcess?.(); + // Send SIGTERM and wait for it to gracefully close. + await this._killProcess?.(this._launchTerminateTimeout); } private async _startProcess(): Promise { @@ -122,15 +125,14 @@ export class WebServerPlugin implements TestRunnerPlugin { } private async _waitForAvailability() { - const launchTimeout = this._options.timeout || 60 * 1000; const cancellationToken = { canceled: false }; const { timedOut } = (await Promise.race([ - raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), launchTimeout), + raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), this._launchTerminateTimeout), this._processExitedPromise, ])); cancellationToken.canceled = true; if (timedOut) - throw new Error(`Timed out waiting ${launchTimeout}ms from config.webServer.`); + throw new Error(`Timed out waiting ${this._launchTerminateTimeout}ms from config.webServer.`); } } diff --git a/packages/playwright-test/types/test.d.ts b/packages/playwright-test/types/test.d.ts index 16da91372c..0e3061debb 100644 --- a/packages/playwright-test/types/test.d.ts +++ b/packages/playwright-test/types/test.d.ts @@ -4599,7 +4599,8 @@ interface TestConfigWebServer { ignoreHTTPSErrors?: boolean; /** - * How long to wait for the process to start up and be available in milliseconds. Defaults to 60000. + * How long to wait for the process to start up and be available in milliseconds. The same timeout is also used to + * terminate the process. Defaults to 60000. */ timeout?: number; diff --git a/tests/playwright-test/assets/simple-server-ignores-sigterm.js b/tests/playwright-test/assets/simple-server-ignores-sigterm.js new file mode 100644 index 0000000000..209f4c98bf --- /dev/null +++ b/tests/playwright-test/assets/simple-server-ignores-sigterm.js @@ -0,0 +1,13 @@ +const http = require('http'); + +const port = process.argv[2] || 3000; + +const server = http.createServer(function (req, res) { + res.end('running!'); +}); +process.on('SIGTERM', () => console.log('received SIGTERM - ignoring')); +process.on('SIGINT', () => console.log('received SIGINT - ignoring')); + +server.listen(port, () => { + console.log('listening on port', port); +}); diff --git a/tests/playwright-test/web-server.spec.ts b/tests/playwright-test/web-server.spec.ts index e70b197b4d..3fc8c0a774 100644 --- a/tests/playwright-test/web-server.spec.ts +++ b/tests/playwright-test/web-server.spec.ts @@ -19,6 +19,7 @@ import path from 'path'; import { test, expect } from './playwright-test-fixtures'; const SIMPLE_SERVER_PATH = path.join(__dirname, 'assets', 'simple-server.js'); +const SIMPLE_SERVER_THAT_IGNORES_SIGTERM_PATH = path.join(__dirname, 'assets', 'simple-server-ignores-sigterm.js'); test('should create a server', async ({ runInlineTest }, { workerIndex }) => { const port = workerIndex + 10500; @@ -601,3 +602,27 @@ test('should treat 3XX as available server', async ({ runInlineTest }, { workerI expect(result.output).toContain('[WebServer] listening'); expect(result.output).toContain('[WebServer] error from server'); }); + +test('should be able to kill process that ignores SIGTERM', async ({ runInlineTest }, { workerIndex }) => { + test.skip(process.platform === 'win32', 'there is no SIGTERM on Windows'); + const port = workerIndex + 10500; + const result = await runInlineTest({ + 'test.spec.ts': ` + const { test } = pwt; + test('pass', async ({}) => {}); + `, + 'playwright.config.ts': ` + module.exports = { + webServer: { + command: 'node ${JSON.stringify(SIMPLE_SERVER_THAT_IGNORES_SIGTERM_PATH)} ${port}', + port: ${port}, + timeout: 1000, + } + }; + `, + }, {}, { DEBUG: 'pw:webserver' }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(1); + expect(result.output).toContain('[WebServer] listening'); + expect(result.output).toContain('[WebServer] received SIGTERM - ignoring'); +});