feat(rpc): gracefully close browsers in server process on disconnect (#3005)

Author: dgozman

src/rpc/server.ts

@@ -20,12 +20,19 @@ import { Playwright } from '../server/playwright';
 import { PlaywrightDispatcher } from './server/playwrightDispatcher';
 import { Electron } from '../server/electron';
 import { setUseApiName } from '../progress';
+import { gracefullyCloseAll } from '../server/processLauncher';
 
 setUseApiName(false);
 
 const dispatcherConnection = new DispatcherConnection();
 const transport = new Transport(process.stdout, process.stdin);
-transport.onclose = () => process.exit(0);
+transport.onclose = async () => {
+  // Force exit after 30 seconds.
+  setTimeout(() => process.exit(0), 30000);
+  // Meanwhile, try to gracefully close all browsers.
+  await gracefullyCloseAll();
+  process.exit(0);
+};
 transport.onmessage = message => dispatcherConnection.dispatch(JSON.parse(message));
 dispatcherConnection.onmessage = message => transport.send(JSON.stringify(message));
 

src/server/processLauncher.ts

@@ -49,6 +49,12 @@ type LaunchResult = {
   kill: () => Promise<void>,
 };
 
+const gracefullyCloseSet = new Set<() => Promise<void>>();
+
+export async function gracefullyCloseAll() {
+  await Promise.all(Array.from(gracefullyCloseSet).map(gracefullyClose => gracefullyClose().catch(e => {})));
+}
+
 export async function launchProcess(options: LaunchProcessOptions): Promise<LaunchResult> {
   const cleanup = () => helper.removeFolders(options.tempDirectories);
 
@@ -97,6 +103,7 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
     progress.logger.info(`<process did exit: exitCode=${exitCode}, signal=${signal}>`);
     processClosed = true;
     helper.removeEventListeners(listeners);
+    gracefullyCloseSet.delete(gracefullyClose);
     options.onExit(exitCode, signal);
     fulfillClose();
     // Cleanup as process exits.
@@ -113,9 +120,11 @@ export async function launchProcess(options: LaunchProcessOptions): Promise<Laun
     listeners.push(helper.addEventListener(process, 'SIGTERM', gracefullyClose));
   if (options.handleSIGHUP)
     listeners.push(helper.addEventListener(process, 'SIGHUP', gracefullyClose));
+  gracefullyCloseSet.add(gracefullyClose);
 
   let gracefullyClosing = false;
   async function gracefullyClose(): Promise<void> {
+    gracefullyCloseSet.delete(gracefullyClose);
     // 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, for example user sends SIGINT second time.

test/chromium/launcher.jest.js

@@ -32,7 +32,7 @@ describe.skip(!CHROMIUM)('launcher', function() {
     const browser = await browserType.launchServer(options);
     await browser.close();
   });
-  it.skip(USES_HOOKS).fail(CHROMIUM && WIN)('should open devtools when "devtools: true" option is given', async({browserType, defaultBrowserOptions}) => {
+  it.fail(USES_HOOKS || WIN)('should open devtools when "devtools: true" option is given', async({browserType, defaultBrowserOptions}) => {
     let devtoolsCallback;
     const devtoolsPromise = new Promise(f => devtoolsCallback = f);
     const __testHookForDevTools = devtools => devtools.__testHookOnBinding = parsed => {

test/environments.js

@@ -161,7 +161,7 @@ class PlaywrightEnvironment {
   constructor(playwright) {
     this._playwright = playwright;
     this.spawnedProcess = undefined;
-    this.expectExit = false;
+    this.onExit = undefined;
   }
 
   name() { return 'Playwright'; };
@@ -173,13 +173,13 @@ class PlaywrightEnvironment {
       if (process.env.PWCHANNEL === 'wire') {
         this.spawnedProcess = childProcess.fork(path.join(__dirname, '..', 'lib', 'rpc', 'server'), [], {
           stdio: 'pipe',
-          detached: process.platform !== 'win32',
-        });
-        this.spawnedProcess.once('exit', (exitCode, signal) => {
-          this.spawnedProcess = undefined;
-          if (!this.expectExit)
-            throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`);
+          detached: true,
         });
+        this.spawnedProcess.unref();
+        this.onExit = (exitCode, signal) => {
+          throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`);
+        };
+        this.spawnedProcess.once('exit', this.onExit);
         const transport = new Transport(this.spawnedProcess.stdin, this.spawnedProcess.stdout);
         connection.onmessage = message => transport.send(JSON.stringify(message));
         transport.onmessage = message => connection.dispatch(JSON.parse(message));
@@ -205,10 +205,10 @@ class PlaywrightEnvironment {
 
   async afterAll(state) {
     if (this.spawnedProcess) {
-      const exited = new Promise(f => this.spawnedProcess.once('exit', f));
-      this.expectExit = true;
-      this.spawnedProcess.kill();
-      await exited;
+      this.spawnedProcess.removeListener('exit', this.onExit);
+      this.spawnedProcess.stdin.destroy();
+      this.spawnedProcess.stdout.destroy();
+      this.spawnedProcess.stderr.destroy();
     }
     delete state.playwright;
   }

test/jest/fixtures.js

@@ -40,21 +40,21 @@ module.exports = function registerFixtures(global) {
     server.PREFIX = `http://localhost:${port}`;
     server.CROSS_PROCESS_PREFIX = `http://127.0.0.1:${port}`;
     server.EMPTY_PAGE = `http://localhost:${port}/empty.html`;
-  
+
     const httpsPort = port + 1;
     const httpsServer = await TestServer.createHTTPS(assetsPath, httpsPort);
     httpsServer.enableHTTPCache(cachedPath);
     httpsServer.PORT = httpsPort;
     httpsServer.PREFIX = `https://localhost:${httpsPort}`;
     httpsServer.CROSS_PROCESS_PREFIX = `https://127.0.0.1:${httpsPort}`;
     httpsServer.EMPTY_PAGE = `https://localhost:${httpsPort}/empty.html`;
-  
+
     await test({server, httpsServer});
 
     await Promise.all([
       server.stop(),
       httpsServer.stop(),
-    ]);  
+    ]);
   });
 
   global.registerWorkerFixture('defaultBrowserOptions', async({}, test) => {
@@ -72,17 +72,17 @@ module.exports = function registerFixtures(global) {
       const connection = new Connection();
       let toImpl;
       let spawnedProcess;
-      let expectExit;
+      let onExit;
       if (process.env.PWCHANNEL === 'wire') {
         spawnedProcess = childProcess.fork(path.join(__dirname, '..', '..', 'lib', 'rpc', 'server'), [], {
           stdio: 'pipe',
-          detached: process.platform !== 'win32',
-        });
-        spawnedProcess.once('exit', (exitCode, signal) => {
-          spawnedProcess = undefined;
-          if (!expectExit)
-            throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`);
+          detached: true,
         });
+        spawnedProcess.unref();
+        onExit = (exitCode, signal) => {
+          throw new Error(`Server closed with exitCode=${exitCode} signal=${signal}`);
+        };
+        spawnedProcess.on('exit', onExit);
         const transport = new Transport(spawnedProcess.stdin, spawnedProcess.stdout);
         connection.onmessage = message => transport.send(JSON.stringify(message));
         transport.onmessage = message => connection.dispatch(JSON.parse(message));
@@ -103,17 +103,16 @@ module.exports = function registerFixtures(global) {
       const playwrightObject = await connection.waitForObjectWithKnownName('playwright');
       playwrightObject.toImpl = toImpl;
       await test(playwrightObject);
-
       if (spawnedProcess) {
-        const exited = new Promise(f => spawnedProcess.once('exit', f));
-        expectExit = true;
-        spawnedProcess.kill();
-        await exited;
+        spawnedProcess.removeListener('exit', onExit);
+        spawnedProcess.stdin.destroy();
+        spawnedProcess.stdout.destroy();
+        spawnedProcess.stderr.destroy();
       }
-      return;
+    } else {
+      playwright.toImpl = x => x;
+      await test(playwright);
     }
-    playwright.toImpl = x => x;
-    await test(playwright);
   });
 
   global.registerFixture('toImpl', async ({playwright}, test) => {
@@ -123,7 +122,7 @@ module.exports = function registerFixtures(global) {
   global.registerWorkerFixture('browserType', async ({playwright}, test) => {
     await test(playwright[process.env.BROWSER || 'chromium']);
   });
-  
+
   global.registerWorkerFixture('browser', async ({browserType, defaultBrowserOptions}, test) => {
     const browser = await browserType.launch(defaultBrowserOptions);
     try {
@@ -136,7 +135,7 @@ module.exports = function registerFixtures(global) {
       await browser.close();
     }
   });
-  
+
   global.registerFixture('context', async ({browser}, test) => {
     const context = await browser.newContext();
     try {
@@ -145,7 +144,7 @@ module.exports = function registerFixtures(global) {
       await context.close();
     }
   });
-  
+
   global.registerFixture('page', async ({context}, test) => {
     const page = await context.newPage();
     await test(page);

test/jest/playwrightEnvironment.js

@@ -81,6 +81,7 @@ class PlaywrightEnvironment extends NodeEnvironment {
       this.global.describe.fail = this.global.describe.skip;
 
       const itSkip = this.global.it.skip;
+      itSkip.slow = () => itSkip;
       this.global.it.skip = (...args) => {
         if (args.length = 1)
           return args[0] ? itSkip : this.global.it;
@@ -199,7 +200,7 @@ class FixturePool {
       await this.setupFixture(name);
     const params = {};
     for (const n of names)
-      params[n] = this.instances.get(n).value; 
+      params[n] = this.instances.get(n).value;
     return fn(params);
   }
 }

test/launcher.jest.js

@@ -301,7 +301,7 @@ describe('browserType.connect', function() {
     await browserServer._checkLeaks();
     await browserServer.close();
   });
-  it.skip(USES_HOOKS).slow().fail(CHROMIUM && WIN)('should handle exceptions during connect', async({browserType, defaultBrowserOptions, server}) => {
+  it.fail(USES_HOOKS || (CHROMIUM && WIN)).slow()('should handle exceptions during connect', async({browserType, defaultBrowserOptions, server}) => {
     const browserServer = await browserType.launchServer(defaultBrowserOptions);
     const __testHookBeforeCreateBrowser = () => { throw new Error('Dummy') };
     const error = await browserType.connect({ wsEndpoint: browserServer.wsEndpoint(), __testHookBeforeCreateBrowser }).catch(e => e);