From 26044c11c03d13fa076b14a98758d61ede013245 Mon Sep 17 00:00:00 2001
From: Andrey Lushnikov <aslushnikov@gmail.com>
Date: Thu, 10 Nov 2022 22:30:06 -0800
Subject: [PATCH] fix(webserver): do not spawn webserver as a new process group
 (#18564)

This patch 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
-<testrunnerpid>` will orphan web server process.

Instead, this patch simply spawns web server and sends `SIGTERM` to it
when ready.
---
 .../src/plugins/webServerPlugin.ts            | 55 +++++++++++--------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/packages/playwright-test/src/plugins/webServerPlugin.ts b/packages/playwright-test/src/plugins/webServerPlugin.ts
index 6a3f5b5b97ced..d7df9212d681f 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, spawnSync } 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<boolean>;
-  private _killProcess?: () => Promise<void>;
-  private _processExitedPromise!: Promise<any>;
+  private _processExitedPromise?: Promise<{ code: number|null, signal: NodeJS.Signals|null }>;
+  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,27 @@ export class WebServerPlugin implements TestRunnerPlugin {
   }
 
   public async teardown() {
-    await this._killProcess?.();
+    if (!this._childProcess || !this._childProcess.pid || this._childProcess.killed)
+      return;
+    // Send SIGTERM and wait for it to gracefully close.
+    try {
+      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
+    }
+    await this._processExitedPromise;
   }
 
   private async _startProcess(): Promise<void> {
-    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 +103,28 @@ 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, signal) => resolve({ code, signal }));
+    });
 
     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 +135,9 @@ 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(({ code, signal }) => {
+        throw new Error(`Process from config.webServer terminated with exit code "${code}" and signal "${signal}"`);
+      }),
     ]));
     cancellationToken.canceled = true;
     if (timedOut)