exit workers gracefully

CHANGELOG.md

@@ -5,7 +5,9 @@
 - `[babel-plugin-jest-hoist]` Show codeframe on static hoisting issues ([#8865](https://github.com/facebook/jest/pull/8865))
 - `[jest-config]` [**BREAKING**] Set default display name color based on runner ([#8689](https://github.com/facebook/jest/pull/8689))
 - `[jest-diff]` Add options for colors and symbols ([#8841](https://github.com/facebook/jest/pull/8841))
+- `[jest-runner]` Warn if a worker had to be force exited ([#8206](https://github.com/facebook/jest/pull/8206))
 - `[@jest/test-result]` Create method to create empty `TestResult` ([#8867](https://github.com/facebook/jest/pull/8867))
+- `[jest-worker]` [**BREAKING**] Return a promise from `end()`, resolving with the information whether workers exited gracefully ([#8206](https://github.com/facebook/jest/pull/8206))
 
 ### Fixes
 

e2e/Utils.ts

@@ -90,6 +90,21 @@ export const writeFiles = (
   });
 };
 
+const NUMBER_OF_TESTS_TO_FORCE_USING_WORKERS = 25;
+/**
+ * Forces Jest to use workers by generating many test files to run.
+ * Slow and modifies the test output. Use sparingly.
+ */
+export const generateTestFilesToForceUsingWorkers = () => {
+  const testFiles = {};
+  for (let i = 0; i <= NUMBER_OF_TESTS_TO_FORCE_USING_WORKERS; i++) {
+    testFiles[`__tests__/test${i}.test.js`] = `
+      test.todo('test ${i}');
+    `;
+  }
+  return testFiles;
+};
+
 export const copyDir = (src: string, dest: string) => {
   const srcStat = fs.lstatSync(src);
   if (srcStat.isDirectory()) {

e2e/__tests__/fatalWorkerError.test.ts

@@ -7,31 +7,28 @@
 
 import * as path from 'path';
 import {tmpdir} from 'os';
-import {cleanup, writeFiles} from '../Utils';
+import {
+  cleanup,
+  generateTestFilesToForceUsingWorkers,
+  writeFiles,
+} from '../Utils';
 import runJest from '../runJest';
 
 const DIR = path.resolve(tmpdir(), 'fatal-worker-error');
 
 beforeEach(() => cleanup(DIR));
 afterAll(() => cleanup(DIR));
 
-const NUMBER_OF_TESTS_TO_FORCE_USING_WORKERS = 25;
-
 test('fails a test that terminates the worker with a fatal error', () => {
   const testFiles = {
+    ...generateTestFilesToForceUsingWorkers(),
     '__tests__/fatalWorkerError.test.js': `
       test('fatal worker error', () => {
         process.exit(134);
       });
     `,
   };
 
-  for (let i = 0; i <= NUMBER_OF_TESTS_TO_FORCE_USING_WORKERS; i++) {
-    testFiles[`__tests__/test${i}.test.js`] = `
-      test('test ${i}', () => {});
-    `;
-  }
-
   writeFiles(DIR, {
     ...testFiles,
     'package.json': '{}',

e2e/__tests__/workerForceExit.test.ts

@@ -0,0 +1,69 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import {tmpdir} from 'os';
+import {resolve} from 'path';
+import findProcess from 'find-process';
+
+import {
+  cleanup,
+  generateTestFilesToForceUsingWorkers,
+  writeFiles,
+} from '../Utils';
+import runJest from '../runJest';
+
+const DIR = resolve(tmpdir(), 'worker-force-exit');
+
+beforeEach(() => cleanup(DIR));
+afterEach(() => cleanup(DIR));
+const testFiles = {
+  ...generateTestFilesToForceUsingWorkers(),
+  'package.json': `{
+      "testEnvironment": "node"
+  }`,
+};
+
+const verifyNumPassed = stderr => {
+  const numberOfTestsPassed = (stderr.match(/\bPASS\b/g) || []).length;
+  // assuming -1 because of package.json, but +1 because of the individual test file
+  expect(numberOfTestsPassed).toBe(Object.keys(testFiles).length);
+};
+
+test('prints a warning if a worker is force exited', () => {
+  writeFiles(DIR, {
+    ...testFiles,
+    '__tests__/simple.test.js': `
+      test('t', () => {
+        require('http').createServer().listen(0);
+      });
+    `,
+  });
+  const {status, stderr, stdout} = runJest(DIR, ['--maxWorkers=2']);
+
+  expect(status).toBe(0);
+  verifyNumPassed(stderr);
+  expect(stdout).toContain('A worker process has failed to exit gracefully');
+});
+
+test('force exits a worker that fails to exit gracefully', async () => {
+  writeFiles(DIR, {
+    ...testFiles,
+    '__tests__/timeoutKilled.test.js': `
+      test('t', () => {
+        require('http').createServer().listen(0);
+        console.error('pid: ' + process.pid);
+      });
+    `,
+  });
+  const {status, stderr} = runJest(DIR, ['--maxWorkers=2']);
+
+  expect(status).toBe(0);
+  verifyNumPassed(stderr);
+
+  const [pid] = /pid: \d+/.exec(stderr);
+  expect(await findProcess('pid', pid)).toHaveLength(0);
+});

package.json

@@ -38,6 +38,7 @@
     "eslint-plugin-react": "^7.1.0",
     "execa": "^2.0.4",
     "fast-check": "^1.13.0",
+    "find-process": "^1.4.1",
     "glob": "^7.1.1",
     "graceful-fs": "^4.1.15",
     "isbinaryfile": "^4.0.0",

packages/jest-runner/src/__tests__/testRunner.test.ts

@@ -15,7 +15,7 @@ jest.mock('jest-worker', () =>
   jest.fn(
     worker =>
       (mockWorkerFarm = {
-        end: jest.fn(),
+        end: jest.fn().mockResolvedValue({forceExited: false}),
         getStderr: jest.fn(),
         getStdout: jest.fn(),
         worker: jest.fn((data, callback) => require(worker)(data, callback)),

packages/jest-runner/src/index.ts

@@ -8,6 +8,7 @@
 import {Config} from '@jest/types';
 import {SerializableError} from '@jest/test-result';
 import exit = require('exit');
+import chalk from 'chalk';
 import throat from 'throat';
 import Worker from 'jest-worker';
 import runTest from './runTest';
@@ -189,7 +190,18 @@ class TestRunner {
       ),
     );
 
-    const cleanup = () => worker.end();
+    const cleanup = async () => {
+      const {forceExited} = await worker.end();
+      if (forceExited) {
+        console.log(
+          chalk.yellow(
+            'A worker process has failed to exit gracefully and has been force exited. ' +
+              'This is likely caused by tests leaking due to improper teardown. ' +
+              'Try running with --runInBand --detectOpenHandles to find leaks.',
+          ),
+        );
+      }
+    };
     return Promise.race([runAllTests, onInterrupt]).then(cleanup, cleanup);
   }
 }

packages/jest-worker/README.md

@@ -109,6 +109,8 @@ Returns a `ReadableStream` where the standard error of all workers is piped. Not
 
 Finishes the workers by killing all workers. No further calls can be done to the `Worker` instance.
 
+Returns a Promise that resolves with `{ forceExited: boolean }` once all workers are dead. If `forceExited` is `true`, at least one of the workers did not exit gracefully, which likely happened because it executed a leaky task that left handles open. This should be avoided, force exiting workers is a last resort to prevent creating lots of orphans.
+
 **Note:** Each worker has a unique id (index that starts with `1`) which is available on `process.env.JEST_WORKER_ID`
 
 ## Setting up and tearing down the child process
@@ -139,7 +141,10 @@ async function main() {
   console.log(await myWorker.bar('Bob')); // "Hello from bar: Bob"
   console.log(await myWorker.getWorkerId()); // "3" -> this message has sent from the 3rd worker
 
-  myWorker.end();
+  const {forceExited} = await myWorker.end();
+  if (forceExited) {
+    console.error('Workers failed to exit gracefully');
+  }
 }
 
 main();
@@ -186,7 +191,10 @@ async function main() {
   // the same worker that processed the file the first time will process it now.
   console.log(await myWorker.transform('/tmp/foo.js'));
 
-  myWorker.end();
+  const {forceExited} = await myWorker.end();
+  if (forceExited) {
+    console.error('Workers failed to exit gracefully');
+  }
 }
 
 main();

packages/jest-worker/src/__tests__/index.test.js

@@ -133,18 +133,18 @@ it('does not let make calls after the farm is ended', () => {
   );
 });
 
-it('does not let end the farm after it is ended', () => {
+it('does not let end the farm after it is ended', async () => {
   const farm = new Farm('/tmp/baz.js', {
     exposedMethods: ['foo', 'bar'],
     numWorkers: 4,
   });
 
   farm.end();
   expect(farm._workerPool.end).toHaveBeenCalledTimes(1);
-  expect(() => farm.end()).toThrow(
+  await expect(farm.end()).rejects.toThrow(
     'Farm is ended, no more calls can be done to it',
   );
-  expect(() => farm.end()).toThrow(
+  await expect(farm.end()).rejects.toThrow(
     'Farm is ended, no more calls can be done to it',
   );
   expect(farm._workerPool.end).toHaveBeenCalledTimes(1);

packages/jest-worker/src/base/BaseWorkerPool.ts

@@ -10,11 +10,16 @@ import mergeStream = require('merge-stream');
 
 import {
   CHILD_MESSAGE_END,
+  PoolExitResult,
   WorkerInterface,
   WorkerOptions,
   WorkerPoolOptions,
 } from '../types';
 
+// How long to wait for the child process to terminate
+// after CHILD_MESSAGE_END before sending force exiting.
+const FORCE_EXIT_DELAY = 500;
+
 /* istanbul ignore next */
 const emptyMethod = () => {};
 
@@ -85,15 +90,33 @@ export default class BaseWorkerPool {
     throw Error('Missing method createWorker in WorkerPool');
   }
 
-  end(): void {
+  async end(): Promise<PoolExitResult> {
     // We do not cache the request object here. If so, it would only be only
     // processed by one of the workers, and we want them all to close.
-    for (let i = 0; i < this._workers.length; i++) {
-      this._workers[i].send(
-        [CHILD_MESSAGE_END, false],
-        emptyMethod,
-        emptyMethod,
-      );
-    }
+    const workerExitPromises = this._workers.map(async worker => {
+      worker.send([CHILD_MESSAGE_END, false], emptyMethod, emptyMethod);
+
+      // Schedule a force exit in case worker fails to exit gracefully so
+      // await worker.waitForExit() never takes longer than FORCE_EXIT_DELAY
+      let forceExited = false;
+      const forceExitTimeout = setTimeout(() => {
+        worker.forceExit();
+        forceExited = true;
+      }, FORCE_EXIT_DELAY);
+
+      await worker.waitForExit();
+      // Worker ideally exited gracefully, don't send force exit then
+      clearTimeout(forceExitTimeout);
+
+      return forceExited;
+    });
+
+    const workerExits = await Promise.all(workerExitPromises);
+    return workerExits.reduce<PoolExitResult>(
+      (result, forceExited) => ({
+        forceExited: result.forceExited || forceExited,
+      }),
+      {forceExited: false},
+    );
   }
 }