From c4f17e727e6eae2c950bdb4249f0577f3c91e0e6 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Fri, 4 Nov 2022 00:34:44 -0700 Subject: [PATCH 1/4] fix(webserver): do not spawn webserver as a new process group This patch: - reverts c63a0b536d1f0119794a909e4f9c420c8506b4d5. - stops using `processLauncher` to launch web servers. Process Launcher will spawn a new process group which is separate from test runner. This might result in unexpected behavior, e.g. `kill -sigkill -` will orphane web server process. Instead, this patch simply spawns web server and sends `SIGTERM` to it when ready. --- .../src/plugins/webServerPlugin.ts | 51 +++++++++++-------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts index 6a3f5b5b97ced..420aa33c34042 100644 --- a/packages/playwright-test/src/plugins/webServerPlugin.ts +++ b/packages/playwright-test/src/plugins/webServerPlugin.ts @@ -17,10 +17,11 @@ import http from 'http'; import https from 'https'; import path from 'path'; import net from 'net'; +import { spawn, execSync } from 'child_process'; +import type { ChildProcess } from 'child_process'; import { debug } from 'playwright-core/lib/utilsBundle'; import { raceAgainstTimeout } from 'playwright-core/lib/utils/timeoutRunner'; -import { launchProcess } from 'playwright-core/lib/utils/processLauncher'; import type { FullConfig, Reporter, Suite } from '../../types/testReporter'; import type { TestRunnerPlugin } from '.'; @@ -46,8 +47,8 @@ const debugWebServer = debug('pw:webserver'); export class WebServerPlugin implements TestRunnerPlugin { private _isAvailable?: () => Promise; - private _killProcess?: () => Promise; - private _processExitedPromise!: Promise; + private _processExitedPromise?: Promise; + private _childProcess?: ChildProcess; private _options: WebServerPluginOptions; private _checkPortOnly: boolean; private _reporter?: Reporter; @@ -64,7 +65,6 @@ export class WebServerPlugin implements TestRunnerPlugin { this._options.cwd = this._options.cwd ? path.resolve(configDir, this._options.cwd) : configDir; try { await this._startProcess(); - await this._waitForProcess(); } catch (error) { await this.teardown(); throw error; @@ -72,13 +72,20 @@ export class WebServerPlugin implements TestRunnerPlugin { } public async teardown() { - await this._killProcess?.(); + if (!this._childProcess || !this._childProcess.pid || this._childProcess.killed) + return; + try { + if (process.platform === 'win32') + execSync(`taskkill /pid ${this._childProcess.pid} /T /F /FI "MEMUSAGE gt 0"`, { stdio: 'ignore' }); + else + process.kill(this._childProcess.pid, 'SIGTERM'); + } catch (e) { + // the process might have already stopped + } + await this._processExitedPromise; } private async _startProcess(): Promise { - let processExitedReject = (error: Error) => { }; - this._processExitedPromise = new Promise((_, reject) => processExitedReject = reject); - const isAlreadyAvailable = await this._isAvailable!(); if (isAlreadyAvailable) { debugWebServer(`WebServer is already available`); @@ -89,33 +96,33 @@ export class WebServerPlugin implements TestRunnerPlugin { } debugWebServer(`Starting WebServer process ${this._options.command}...`); - const { launchedProcess, kill } = await launchProcess({ - command: this._options.command, + + this._childProcess = spawn(this._options.command, [], { env: { ...DEFAULT_ENVIRONMENT_VARIABLES, ...envWithoutExperimentalLoaderOptions(), ...this._options.env, }, cwd: this._options.cwd, - stdio: 'stdin', + stdio: 'pipe', shell: true, - attemptToGracefullyClose: async () => {}, - 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: [], }); - this._killProcess = kill; + this._processExitedPromise = new Promise((resolve, reject) => { + this._childProcess!.once('exit', code => { + if (code) + reject(new Error(`Process from config.webServer was not able to start. Exit code: ${code}`)); + else + resolve(undefined); + }); + }); debugWebServer(`Process started`); - launchedProcess.stderr!.on('data', line => this._reporter!.onStdErr?.('[WebServer] ' + line.toString())); - launchedProcess.stdout!.on('data', line => { + this._childProcess.stderr!.on('data', line => this._reporter!.onStdErr?.('[WebServer] ' + line.toString())); + this._childProcess.stdout!.on('data', line => { if (debugWebServer.enabled) this._reporter!.onStdOut?.('[WebServer] ' + line.toString()); }); - } - - private async _waitForProcess() { debugWebServer(`Waiting for availability...`); await this._waitForAvailability(); debugWebServer(`WebServer available`); @@ -126,7 +133,7 @@ export class WebServerPlugin implements TestRunnerPlugin { const cancellationToken = { canceled: false }; const { timedOut } = (await Promise.race([ raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), launchTimeout), - this._processExitedPromise, + this._processExitedPromise!.then(() => { throw new Error('Process exited early'); }), ])); cancellationToken.canceled = true; if (timedOut) From 29ebd05a864f151254915d6b786ad4b1bfd38ff8 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Wed, 9 Nov 2022 13:37:06 -0800 Subject: [PATCH 2/4] address comments --- packages/playwright-test/src/plugins/webServerPlugin.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts index 420aa33c34042..0d5af851e31ac 100644 --- a/packages/playwright-test/src/plugins/webServerPlugin.ts +++ b/packages/playwright-test/src/plugins/webServerPlugin.ts @@ -47,7 +47,7 @@ const debugWebServer = debug('pw:webserver'); export class WebServerPlugin implements TestRunnerPlugin { private _isAvailable?: () => Promise; - private _processExitedPromise?: Promise; + private _processExitedPromise?: Promise; private _childProcess?: ChildProcess; private _options: WebServerPluginOptions; private _checkPortOnly: boolean; @@ -74,6 +74,7 @@ export class WebServerPlugin implements TestRunnerPlugin { public async teardown() { if (!this._childProcess || !this._childProcess.pid || this._childProcess.killed) return; + // Send SIGTERM and wait for it to gracefully close. try { if (process.platform === 'win32') execSync(`taskkill /pid ${this._childProcess.pid} /T /F /FI "MEMUSAGE gt 0"`, { stdio: 'ignore' }); @@ -108,9 +109,9 @@ export class WebServerPlugin implements TestRunnerPlugin { shell: true, }); this._processExitedPromise = new Promise((resolve, reject) => { - this._childProcess!.once('exit', code => { + this._childProcess!.once('exit', (code, signal) => { if (code) - reject(new Error(`Process from config.webServer was not able to start. Exit code: ${code}`)); + reject(new Error(`Process from config.webServer terminated with exit code "${code}" and signal "${signal}"`)); else resolve(undefined); }); From 9fd07d8f5500ec5d9ddbb444c187885d1b2646dd Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Thu, 10 Nov 2022 16:20:54 -0800 Subject: [PATCH 3/4] fix win32 --- .../playwright-test/src/plugins/webServerPlugin.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts index 0d5af851e31ac..5098f933a6878 100644 --- a/packages/playwright-test/src/plugins/webServerPlugin.ts +++ b/packages/playwright-test/src/plugins/webServerPlugin.ts @@ -17,7 +17,7 @@ import http from 'http'; import https from 'https'; import path from 'path'; import net from 'net'; -import { spawn, execSync } from 'child_process'; +import { spawn, execSync, spawnSync } from 'child_process'; import type { ChildProcess } from 'child_process'; import { debug } from 'playwright-core/lib/utilsBundle'; @@ -76,10 +76,16 @@ export class WebServerPlugin implements TestRunnerPlugin { return; // Send SIGTERM and wait for it to gracefully close. try { - if (process.platform === 'win32') - execSync(`taskkill /pid ${this._childProcess.pid} /T /F /FI "MEMUSAGE gt 0"`, { stdio: 'ignore' }); - else + if (process.platform === 'win32') { + const taskkillProcess = spawnSync(`taskkill /pid ${this._childProcess.pid} /T /F`, { shell: true }); + const [stdout, stderr] = [taskkillProcess.stdout.toString(), taskkillProcess.stderr.toString()]; + if (stdout) + debugWebServer(`[pid=${this._childProcess.pid}] taskkill stdout: ${stdout}`); + if (stderr) + debugWebServer(`[pid=${this._childProcess.pid}] taskkill stderr: ${stderr}`); + } else { process.kill(this._childProcess.pid, 'SIGTERM'); + } } catch (e) { // the process might have already stopped } From 9b882d43782dab22344a4842a7ad7ab5e883858f Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Thu, 10 Nov 2022 17:54:24 -0800 Subject: [PATCH 4/4] fix win --- .../src/plugins/webServerPlugin.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts index 5098f933a6878..d7df9212d681f 100644 --- a/packages/playwright-test/src/plugins/webServerPlugin.ts +++ b/packages/playwright-test/src/plugins/webServerPlugin.ts @@ -17,7 +17,7 @@ import http from 'http'; import https from 'https'; import path from 'path'; import net from 'net'; -import { spawn, execSync, spawnSync } from 'child_process'; +import { spawn, spawnSync } from 'child_process'; import type { ChildProcess } from 'child_process'; import { debug } from 'playwright-core/lib/utilsBundle'; @@ -47,7 +47,7 @@ const debugWebServer = debug('pw:webserver'); export class WebServerPlugin implements TestRunnerPlugin { private _isAvailable?: () => Promise; - private _processExitedPromise?: Promise; + private _processExitedPromise?: Promise<{ code: number|null, signal: NodeJS.Signals|null }>; private _childProcess?: ChildProcess; private _options: WebServerPluginOptions; private _checkPortOnly: boolean; @@ -115,12 +115,7 @@ export class WebServerPlugin implements TestRunnerPlugin { shell: true, }); this._processExitedPromise = new Promise((resolve, reject) => { - this._childProcess!.once('exit', (code, signal) => { - if (code) - reject(new Error(`Process from config.webServer terminated with exit code "${code}" and signal "${signal}"`)); - else - resolve(undefined); - }); + this._childProcess!.once('exit', (code, signal) => resolve({ code, signal })); }); debugWebServer(`Process started`); @@ -140,7 +135,9 @@ export class WebServerPlugin implements TestRunnerPlugin { const cancellationToken = { canceled: false }; const { timedOut } = (await Promise.race([ raceAgainstTimeout(() => waitFor(this._isAvailable!, cancellationToken), launchTimeout), - this._processExitedPromise!.then(() => { throw new Error('Process exited early'); }), + this._processExitedPromise!.then(({ code, signal }) => { + throw new Error(`Process from config.webServer terminated with exit code "${code}" and signal "${signal}"`); + }), ])); cancellationToken.canceled = true; if (timedOut)