feat(launcher): gracefully close browser on sigint (#650)

src/server/chromium.ts

@@ -116,9 +116,9 @@ export class Chromium implements BrowserType {
         const message = { method: 'Browser.close', id: kBrowserCloseMessageId };
         transport.send(JSON.stringify(message));
       },
-      onkill: () => {
+      onkill: (exitCode, signal) => {
         if (browserApp)
-          browserApp.emit(Events.BrowserApp.Close);
+          browserApp.emit(Events.BrowserApp.Close, exitCode, signal);
       },
     });
 

src/server/firefox.ts

@@ -115,9 +115,9 @@ export class Firefox implements BrowserType {
         const message = { method: 'Browser.close', params: {}, id: kBrowserCloseMessageId };
         transport.send(JSON.stringify(message));
       },
-      onkill: () => {
+      onkill: (exitCode, signal) => {
         if (browserApp)
-          browserApp.emit(Events.BrowserApp.Close);
+          browserApp.emit(Events.BrowserApp.Close, exitCode, signal);
       },
     });
 

src/server/processLauncher.ts

@@ -40,7 +40,7 @@ export type LaunchProcessOptions = {
 
   // Note: attemptToGracefullyClose should reject if it does not close the browser.
   attemptToGracefullyClose: () => Promise<any>,
-  onkill: () => void,
+  onkill: (exitCode: number | null, signal: string | null) => void,
 };
 
 type LaunchResult = { launchedProcess: childProcess.ChildProcess, gracefullyClose: () => Promise<void> };
@@ -86,10 +86,11 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
 
   let processClosed = false;
   const waitForProcessToClose = new Promise((fulfill, reject) => {
-    spawnedProcess.once('exit', () => {
+    spawnedProcess.once('exit', (exitCode, signal) => {
       debugLauncher(`[${id}] <process did exit>`);
       processClosed = true;
       helper.removeEventListeners(listeners);
+      options.onkill(exitCode, signal);
       // Cleanup as processes exit.
       if (options.tempDir) {
         removeFolderAsync(options.tempDir)
@@ -98,33 +99,36 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
       } else {
         fulfill();
       }
-      options.onkill();
     });
   });
 
   const listeners = [ helper.addEventListener(process, 'exit', killProcess) ];
-  if (options.handleSIGINT)
+  if (options.handleSIGINT) {
-    listeners.push(helper.addEventListener(process, 'SIGINT', () => { killProcess(); process.exit(130); }));
+    listeners.push(helper.addEventListener(process, 'SIGINT', () => {
+      gracefullyClose().then(() => process.exit(130));
+    }));
+  }
   if (options.handleSIGTERM)
     listeners.push(helper.addEventListener(process, 'SIGTERM', gracefullyClose));
   if (options.handleSIGHUP)
     listeners.push(helper.addEventListener(process, 'SIGHUP', gracefullyClose));
-  let gracefullyClosing = false;
-  return { launchedProcess: spawnedProcess, gracefullyClose };
 
+  let gracefullyClosing = false;
   async function gracefullyClose(): Promise<void> {
     // We keep listeners until we are done, to handle 'exit' and 'SIGINT' while
     // asynchronously closing to prevent zombie processes. This might introduce
-    // reentrancy to this function.
+    // reentrancy to this function, for example user sends SIGINT second time.
-    if (gracefullyClosing)
+    // In this case, let's forcefully kill the process.
+    if (gracefullyClosing) {
+      debugLauncher(`[${id}] <forecefully close>`);
+      killProcess();
       return;
+    }
     gracefullyClosing = true;
     debugLauncher(`[${id}] <gracefully close start>`);
     options.attemptToGracefullyClose().catch(() => killProcess());
-    // TODO: forcefully kill the process after some timeout.
     await waitForProcessToClose;
     debugLauncher(`[${id}] <gracefully close end>`);
-    helper.removeEventListeners(listeners);
   }
 
   // This method has to be sync to be used as 'exit' event handler.
@@ -148,6 +152,8 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
         removeFolder.sync(options.tempDir);
     } catch (e) { }
   }
+
+  return { launchedProcess: spawnedProcess, gracefullyClose };
 }
 
 export function waitForLine(process: childProcess.ChildProcess, inputStream: stream.Readable, regex: RegExp, timeout: number, timeoutError: TimeoutError): Promise<RegExpMatchArray> {

src/server/webkit.ts

@@ -117,9 +117,9 @@ export class WebKit implements BrowserType {
         const message = JSON.stringify({method: 'Browser.close', params: {}, id: kBrowserCloseMessageId});
         transport.send(message);
       },
-      onkill: () => {
+      onkill: (exitCode, signal) => {
         if (browserApp)
-          browserApp.emit(Events.BrowserApp.Close);
+          browserApp.emit(Events.BrowserApp.Close, exitCode, signal);
       },
     });
 

test/fixtures.spec.js

@@ -16,13 +16,56 @@
  */
 
 const path = require('path');
-const {spawn} = require('child_process');
+const {spawn, execSync} = require('child_process');
 
-module.exports.describe = function({testRunner, expect, product, playwrightPath, FFOX, CHROMIUM, WEBKIT}) {
+module.exports.describe = function({testRunner, expect, product, playwright, playwrightPath, defaultBrowserOptions, WIN, FFOX, CHROMIUM, WEBKIT}) {
   const {describe, xdescribe, fdescribe} = testRunner;
   const {it, fit, xit, dit} = testRunner;
   const {beforeAll, beforeEach, afterAll, afterEach} = testRunner;
 
+  async function testSignal(action) {
+    const options = Object.assign({}, defaultBrowserOptions, {
+      // Disable DUMPIO to cleanly read stdout.
+      dumpio: false,
+      webSocket: true,
+      handleSIGINT: true,
+      handleSIGTERM: true,
+      handleSIGHUP: true,
+    });
+    const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), playwrightPath, product, JSON.stringify(options)]);
+    let wsEndPointCallback;
+    const wsEndPointPromise = new Promise(x => wsEndPointCallback = x);
+    let output = '';
+    let browserExitCode = 'none';
+    let browserSignal = 'none';
+    let browserPid;
+    res.stdout.on('data', data => {
+      output += data;
+      // Uncomment to debug these tests.
+      // console.log(data.toString());
+      let match = output.match(/browserWS:(.+):browserWS/);
+      if (match)
+        wsEndPointCallback(match[1]);
+      match = output.match(/browserClose:([^:]+):([^:]+):browserClose/);
+      if (match) {
+        browserExitCode = match[1];
+        browserSignal = match[2];
+      }
+      match = output.match(/browserPid:([^:]+):browserPid/);
+      if (match)
+        browserPid = +match[1];
+    });
+    res.on('error', (...args) => console.log("ERROR", ...args));
+    const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise });
+    const promises = [
+      new Promise(resolve => browser.once('disconnected', resolve)),
+      new Promise(resolve => res.on('exit', resolve)),
+    ];
+    action(res, browserPid);
+    const [, exitCode] = await Promise.all(promises);
+    return { exitCode, browserSignal, browserExitCode, output };
+  }
+
   describe('Fixtures', function() {
     it('dumpio option should work with webSocket option', async({server}) => {
       let dumpioData = '';
@@ -38,5 +81,82 @@ module.exports.describe = function({testRunner, expect, product, playwrightPath,
       await new Promise(resolve => res.on('close', resolve));
       expect(dumpioData).toContain('message from dumpio');
     });
+    it('should close the browser when the node process closes', async () => {
+      const result = await testSignal(child => {
+        if (process.platform === 'win32')
+          execSync(`taskkill /pid ${child.pid} /T /F`);
+        else
+          process.kill(child.pid);
+      });
+      expect(result.exitCode).toBe(WIN ? 1 : 0);
+      // We might not get browser exitCode in time when killing the parent node process,
+      // so we don't check it here.
+    });
+    if (!WIN) {
+      // Cannot reliably send signals on Windows.
+      it('should report browser close signal', async () => {
+        const result = await testSignal((child, browserPid) => {
+          process.kill(browserPid);
+          process.kill(child.pid, 'SIGINT');
+        });
+        expect(result.exitCode).toBe(130);
+        expect(result.browserExitCode).toBe('null');
+        expect(result.browserSignal).toBe('SIGTERM');
+      });
+      it('should report browser close signal 2', async () => {
+        const result = await testSignal((child, browserPid) => {
+          process.kill(browserPid, 'SIGKILL');
+          process.kill(child.pid, 'SIGINT');
+        });
+        expect(result.exitCode).toBe(130);
+        expect(result.browserExitCode).toBe('null');
+        expect(result.browserSignal).toBe('SIGKILL');
+      });
+      it('should close the browser on SIGINT', async () => {
+        const result = await testSignal(child => process.kill(child.pid, 'SIGINT'));
+        expect(result.exitCode).toBe(130);
+        expect(result.browserExitCode).toBe('0');
+        expect(result.browserSignal).toBe('null');
+      });
+      it('should close the browser on SIGTERM', async () => {
+        const result = await testSignal(child => process.kill(child.pid, 'SIGTERM'));
+        expect(result.exitCode).toBe(0);
+        expect(result.browserExitCode).toBe('0');
+        expect(result.browserSignal).toBe('null');
+      });
+      it('should close the browser on SIGHUP', async () => {
+        const result = await testSignal(child => process.kill(child.pid, 'SIGHUP'));
+        expect(result.exitCode).toBe(0);
+        expect(result.browserExitCode).toBe('0');
+        expect(result.browserSignal).toBe('null');
+      });
+      it('should kill the browser on double SIGINT', async () => {
+        const result = await testSignal(child => {
+          process.kill(child.pid, 'SIGINT');
+          process.kill(child.pid, 'SIGINT');
+        });
+        expect(result.exitCode).toBe(130);
+        // TODO: ideally, we would expect the SIGKILL on the browser from
+        // force kill, but that's racy with sending two signals.
+      });
+      it('should kill the browser on SIGINT + SIGTERM', async () => {
+        const result = await testSignal(child => {
+          process.kill(child.pid, 'SIGINT');
+          process.kill(child.pid, 'SIGTERM');
+        });
+        expect(result.exitCode).toBe(130);
+        // TODO: ideally, we would expect the SIGKILL on the browser from
+        // force kill, but that's racy with sending two signals.
+      });
+      it('should kill the browser on SIGTERM + SIGINT', async () => {
+        const result = await testSignal(child => {
+          process.kill(child.pid, 'SIGTERM');
+          process.kill(child.pid, 'SIGINT');
+        });
+        expect(result.exitCode).toBe(130);
+        // TODO: ideally, we would expect the SIGKILL on the browser from
+        // force kill, but that's racy with sending two signals.
+      });
+    }
   });
 };

test/fixtures/closeme.js

@@ -1,5 +1,9 @@
 (async() => {
   const [, , playwrightRoot, product, options] = process.argv;
   const browserApp = await require(playwrightRoot)[product.toLowerCase()].launchBrowserApp(JSON.parse(options));
-  console.log(browserApp.wsEndpoint());
+  browserApp.on('close', (exitCode, signal) => {
+    console.log(`browserClose:${exitCode}:${signal}:browserClose`);
+  });
+  console.log(`browserPid:${browserApp.process().pid}:browserPid`);
+  console.log(`browserWS:${browserApp.wsEndpoint()}:browserWS`);
 })();

test/launcher.spec.js

@@ -18,7 +18,6 @@ const fs = require('fs');
 const os = require('os');
 const path = require('path');
 const util = require('util');
-const {spawn, execSync} = require('child_process');
 
 const utils = require('./utils');
 const rmAsync = util.promisify(require('rimraf'));
@@ -67,32 +66,6 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p
         expect(page.url()).toBe(server.EMPTY_PAGE);
         await browser.close();
       });
-      it('should close the browser when the node process closes', async({ server }) => {
-        const options = Object.assign({}, defaultBrowserOptions, {
-          // Disable DUMPIO to cleanly read stdout.
-          dumpio: false,
-          webSocket: true,
-        });
-        const res = spawn('node', [path.join(__dirname, 'fixtures', 'closeme.js'), playwrightPath, product, JSON.stringify(options)]);
-        let wsEndPointCallback;
-        const wsEndPointPromise = new Promise(x => wsEndPointCallback = x);
-        let output = '';
-        res.stdout.on('data', data => {
-          output += data;
-          if (output.indexOf('\n'))
-            wsEndPointCallback(output.substring(0, output.indexOf('\n')));
-        });
-        const browser = await playwright.connect({ browserWSEndpoint: await wsEndPointPromise });
-        const promises = [
-          new Promise(resolve => browser.once('disconnected', resolve)),
-          new Promise(resolve => res.on('exit', resolve))
-        ];
-        if (process.platform === 'win32')
-          execSync(`taskkill /pid ${res.pid} /T /F`);
-        else
-          process.kill(res.pid);
-        await Promise.all(promises);
-      });
       it('should return child_process instance', async () => {
         const browserApp = await playwright.launchBrowserApp(defaultBrowserOptions);
         expect(browserApp.process().pid).toBeGreaterThan(0);