From 18f35f820ad0783c5745df41348f6a484984a0a0 Mon Sep 17 00:00:00 2001
From: Simon Knott <info@simonknott.de>
Date: Thu, 31 Oct 2024 11:22:57 +0100
Subject: [PATCH] feat(webserver): send graceful SIGINT before killing

---
 .../playwright/src/plugins/webServerPlugin.ts | 21 +++++++++---
 tests/config/commonFixtures.ts                | 12 +++++--
 tests/playwright-test/web-server.spec.ts      | 34 +++++++++++++++++++
 3 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/packages/playwright/src/plugins/webServerPlugin.ts b/packages/playwright/src/plugins/webServerPlugin.ts
index e2474f35f2..6a13afcad3 100644
--- a/packages/playwright/src/plugins/webServerPlugin.ts
+++ b/packages/playwright/src/plugins/webServerPlugin.ts
@@ -15,6 +15,7 @@
  */
 import path from 'path';
 import net from 'net';
+import timers from 'timers/promises';
 
 import { colors, debug } from 'playwright-core/lib/utilsBundle';
 import { raceAgainstDeadline, launchProcess, monotonicTime, isURLAvailable } from 'playwright-core/lib/utils';
@@ -92,7 +93,7 @@ export class WebServerPlugin implements TestRunnerPlugin {
     }
 
     debugWebServer(`Starting WebServer process ${this._options.command}...`);
-    const { launchedProcess, kill } = await launchProcess({
+    const { launchedProcess, gracefullyClose } = await launchProcess({
       command: this._options.command,
       env: {
         ...DEFAULT_ENVIRONMENT_VARIABLES,
@@ -102,14 +103,24 @@ export class WebServerPlugin implements TestRunnerPlugin {
       cwd: this._options.cwd,
       stdio: 'stdin',
       shell: true,
-      // Reject to indicate that we cannot close the web server gracefully
-      // and should fallback to non-graceful shutdown.
-      attemptToGracefullyClose: () => Promise.reject(),
+      attemptToGracefullyClose: async () => {
+        const success = launchedProcess.kill('SIGINT');
+        if (!success)
+          throw new Error(`SIGINT didn't succeed, fall back to non-graceful shutdown`);
+        await Promise.race([
+          timers.setTimeout(1000).then(() => {
+            // @ts-expect-error. SIGINT didn't kill the process, but `processLauncher` will only attempt killing it if this is false
+            launchedProcess.killed = false;
+            return Promise.reject(new Error(`server didn't close gracefully within a second, falling back to non-graceful shutdown`))
+          }),
+          new Promise(f => launchedProcess.once('exit', f)),
+        ]);
+      },
       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._killProcess = gracefullyClose;
 
     debugWebServer(`Process started`);
 
diff --git a/tests/config/commonFixtures.ts b/tests/config/commonFixtures.ts
index 57f1d76373..fddbd3bd83 100644
--- a/tests/config/commonFixtures.ts
+++ b/tests/config/commonFixtures.ts
@@ -151,9 +151,17 @@ export class TestChildProcess {
     this.exitCode = this.exited.then(r => r.exitCode);
   }
 
-  outputLines(): string[] {
+  outputLines(options: { prefix?: string } = {}): string[] {
     const strippedOutput = stripAnsi(this.output);
-    return strippedOutput.split('\n').filter(line => line.startsWith('%%')).map(line => line.substring(2).trim());
+    return strippedOutput
+        .split('\n')
+        .map(line => {
+          if (options.prefix && line.startsWith(options.prefix))
+            return line.substring(options.prefix.length);
+          return line;
+        })
+        .filter(line => line.startsWith('%%'))
+        .map(line => line.substring(2).trim());
   }
 
   async kill(signal: 'SIGINT' | 'SIGKILL' = 'SIGKILL') {
diff --git a/tests/playwright-test/web-server.spec.ts b/tests/playwright-test/web-server.spec.ts
index 04e3c1d328..8fed59eb0c 100644
--- a/tests/playwright-test/web-server.spec.ts
+++ b/tests/playwright-test/web-server.spec.ts
@@ -744,3 +744,37 @@ test('should forward stdout when set to "pipe" before server is ready', async ({
   expect(result.output).toContain('[WebServer] output from server');
   expect(result.output).not.toContain('Timed out waiting 3000ms');
 });
+
+test('should gracefully kill server', async ({ interactWithTestRunner }, { workerIndex }) => {
+  test.skip(process.platform === 'win32', 'No sending SIGINT on Windows');
+
+  const port = workerIndex * 2 + 10510;
+
+  const testProcess = await interactWithTestRunner({
+    'web-server.js': `
+      process.on('SIGINT', () => { console.log('%%webserver received SIGINT but stubbornly refuses to wind down') })
+      const server = require('http').createServer((req, res) => { res.end("ok"); })
+      server.listen(process.argv[2], () => { console.log('webserver started'); });
+    `,
+    'test.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test('pass', async ({}) => {});
+    `,
+    'playwright.config.ts': `
+      module.exports = {
+        webServer: {
+          command: 'node web-server.js ${port}',
+          port: ${port},
+          stdout: 'pipe',
+          timeout: 3000,
+        },
+      };
+    `,
+  }, { workers: 1 });
+
+  await testProcess.waitForOutput('webserver started');
+  process.kill(-testProcess.process.pid!, 'SIGINT');
+  await testProcess.exited;
+
+  expect(testProcess.outputLines({ prefix: '[WebServer] ' })).toEqual(['webserver received SIGINT but stubbornly refuses to wind down']);
+});