From ae3f617bf2e2e39d191c288dffdbbf86e301bc2d Mon Sep 17 00:00:00 2001 From: Tim Seckinger Date: Mon, 25 Mar 2019 21:24:11 +0100 Subject: [PATCH] process.exit => SIGTERM & SIGKILL --- .../jest-worker/src/base/BaseWorkerPool.ts | 11 ++--- .../src/base/__tests__/BaseWorkerPool.test.js | 20 ++++----- packages/jest-worker/src/types.ts | 4 +- .../src/workers/ChildProcessWorker.ts | 17 +++++++- .../__tests__/ChildProcessWorker.test.js | 41 +++++++++++++++++++ .../workers/__tests__/processChild.test.js | 16 -------- .../jest-worker/src/workers/processChild.ts | 9 ---- 7 files changed, 74 insertions(+), 44 deletions(-) diff --git a/packages/jest-worker/src/base/BaseWorkerPool.ts b/packages/jest-worker/src/base/BaseWorkerPool.ts index 9885c4500a5b..297770c145c8 100644 --- a/packages/jest-worker/src/base/BaseWorkerPool.ts +++ b/packages/jest-worker/src/base/BaseWorkerPool.ts @@ -10,7 +10,6 @@ import mergeStream from 'merge-stream'; import { CHILD_MESSAGE_END, - CHILD_MESSAGE_FORCE_EXIT, PoolExitResult, WorkerPoolOptions, WorkerOptions, @@ -18,8 +17,9 @@ import { } from '../types'; // How long to wait for the child process to terminate -// after CHILD_MESSAGE_END before sending CHILD_MESSAGE_FORCE_EXIT -const FORCE_EXIT_DELAY = 100; +// after CHILD_MESSAGE_END before sending force exiting. +const FORCE_EXIT_DELAY = 500; + /* istanbul ignore next */ const emptyMethod = () => {}; @@ -100,7 +100,7 @@ export default class BaseWorkerPool { // await worker.waitForExit() never takes longer than FORCE_EXIT_DELAY let forceExited = false; const forceExitTimeout = setTimeout(() => { - worker.send([CHILD_MESSAGE_FORCE_EXIT], emptyMethod, emptyMethod); + worker.forceExit(); forceExited = true; }, FORCE_EXIT_DELAY); @@ -111,7 +111,8 @@ export default class BaseWorkerPool { return forceExited; }); - return (await Promise.all(workerExitPromises)).reduce( + const workerExits = await Promise.all(workerExitPromises); + return workerExits.reduce( (result, forceExited) => ({ forceExited: result.forceExited || forceExited, }), diff --git a/packages/jest-worker/src/base/__tests__/BaseWorkerPool.test.js b/packages/jest-worker/src/base/__tests__/BaseWorkerPool.test.js index cc3789b891a2..c249c0a2bd2c 100644 --- a/packages/jest-worker/src/base/__tests__/BaseWorkerPool.test.js +++ b/packages/jest-worker/src/base/__tests__/BaseWorkerPool.test.js @@ -7,7 +7,7 @@ 'use strict'; -import {CHILD_MESSAGE_END, CHILD_MESSAGE_FORCE_EXIT} from '../../types'; +import {CHILD_MESSAGE_END} from '../../types'; import BaseWorkerPool from '../BaseWorkerPool'; @@ -28,6 +28,7 @@ describe('BaseWorkerPool', () => { beforeEach(() => { Worker.mockClear(); Worker.mockImplementation(() => ({ + forceExit: jest.fn(), getStderr: () => ({once() {}, pipe() {}}), getStdout: () => ({once() {}, pipe() {}}), send: jest.fn(), @@ -227,6 +228,7 @@ describe('BaseWorkerPool', () => { it('resolves with forceExited=false if workers exited gracefully', async () => { Worker.mockImplementation(() => ({ + forceExit: jest.fn(), getStderr: () => null, getStdout: () => null, send: jest.fn(), @@ -245,18 +247,18 @@ describe('BaseWorkerPool', () => { it('force exits workers that do not exit gracefully and resolves with forceExited=true', async () => { // Set it up so that the first worker does not resolve waitForExit immediately, - // but only when it receives CHILD_MESSAGE_FORCE_EXIT + // but only when forceExit() is called let worker0Exited; Worker.mockImplementationOnce(() => ({ + forceExit: () => { + worker0Exited(); + }, getStderr: () => null, getStdout: () => null, - send: request => { - if (request[0] === CHILD_MESSAGE_FORCE_EXIT) { - worker0Exited(); - } - }, + send: jest.fn(), waitForExit: () => new Promise(resolve => (worker0Exited = resolve)), })).mockImplementation(() => ({ + forceExit: jest.fn(), getStderr: () => null, getStdout: () => null, send: jest.fn(), @@ -273,9 +275,7 @@ describe('BaseWorkerPool', () => { const workers = pool.getWorkers(); expect(await pool.end()).toEqual({forceExited: true}); - expect(workers[1].send).not.toHaveBeenCalledWith([ - CHILD_MESSAGE_FORCE_EXIT, - ]); + expect(workers[1].forceExit).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/jest-worker/src/types.ts b/packages/jest-worker/src/types.ts index 58ee2c097ee0..98dca2873a61 100644 --- a/packages/jest-worker/src/types.ts +++ b/packages/jest-worker/src/types.ts @@ -15,7 +15,6 @@ import {ForkOptions} from 'child_process'; export const CHILD_MESSAGE_INITIALIZE: 0 = 0; export const CHILD_MESSAGE_CALL: 1 = 1; export const CHILD_MESSAGE_END: 2 = 2; -export const CHILD_MESSAGE_FORCE_EXIT: 3 = 3; export const PARENT_MESSAGE_OK: 0 = 0; export const PARENT_MESSAGE_CLIENT_ERROR: 1 = 1; @@ -46,6 +45,7 @@ export interface WorkerInterface { onProcessEnd: OnEnd, ): void; waitForExit(): Promise; + forceExit(): void; getWorkerId(): number; getStderr(): NodeJS.ReadableStream | null; @@ -121,8 +121,6 @@ export type ChildMessageEnd = [ boolean // processed ]; -export type ChildMessageForceExit = [typeof CHILD_MESSAGE_FORCE_EXIT]; - export type ChildMessage = | ChildMessageInitialize | ChildMessageCall diff --git a/packages/jest-worker/src/workers/ChildProcessWorker.ts b/packages/jest-worker/src/workers/ChildProcessWorker.ts index 22ac4e745b04..6aaa8b94c10f 100644 --- a/packages/jest-worker/src/workers/ChildProcessWorker.ts +++ b/packages/jest-worker/src/workers/ChildProcessWorker.ts @@ -23,6 +23,9 @@ import { ParentMessage, } from '../types'; +// How long to wait after SIGTERM before sending SIGKILL +const SIGKILL_DELAY = 500; + /** * This class wraps the child process and provides a nice interface to * communicate with. It takes care of: @@ -193,7 +196,10 @@ export default class ChildProcessWorker implements WorkerInterface { } private _onExit(exitCode: number) { - if (exitCode !== 0) { + if ( + exitCode !== 0 && + exitCode <= 128 // importantly, do not reinitialize for 137 (SIGKILL) or 143 (SIGTERM) + ) { this.initialize(); if (this._request) { @@ -222,6 +228,15 @@ export default class ChildProcessWorker implements WorkerInterface { return this._exitPromise; } + forceExit() { + this._child.kill('SIGTERM'); + const sigkillTimeout = setTimeout( + () => this._child.kill('SIGKILL'), + SIGKILL_DELAY, + ); + this._exitPromise.then(() => clearTimeout(sigkillTimeout)); + } + getWorkerId() { return this._options.workerId; } diff --git a/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js b/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js index d5344972234d..47307f9eba37 100644 --- a/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js +++ b/packages/jest-worker/src/workers/__tests__/ChildProcessWorker.test.js @@ -19,6 +19,8 @@ import { PARENT_MESSAGE_OK, } from '../../types'; +jest.useFakeTimers(); + let Worker; let forkInterface; let childProcess; @@ -31,6 +33,7 @@ beforeEach(() => { childProcess = require('child_process'); childProcess.fork.mockImplementation(() => { forkInterface = Object.assign(new EventEmitter(), { + kill: jest.fn(), send: jest.fn(), stderr: new PassThrough(), stdout: new PassThrough(), @@ -318,3 +321,41 @@ it('restarts the child when the child process dies', () => { forkInterface.emit('exit', 1); expect(childProcess.fork).toHaveBeenCalledTimes(2); }); + +it('sends SIGTERM when forceExit() is called', async () => { + const worker = new Worker({ + forkOptions: {}, + maxRetries: 3, + workerPath: '/tmp/foo', + }); + + worker.forceExit(); + expect(forkInterface.kill.mock.calls).toEqual([['SIGTERM']]); +}); + +it('sends SIGKILL some time after SIGTERM', async () => { + const worker = new Worker({ + forkOptions: {}, + maxRetries: 3, + workerPath: '/tmp/foo', + }); + + worker.forceExit(); + jest.runAllTimers(); + expect(forkInterface.kill.mock.calls).toEqual([['SIGTERM'], ['SIGKILL']]); +}); + +it('does not send SIGKILL if SIGTERM exited the process', async () => { + const worker = new Worker({ + forkOptions: {}, + maxRetries: 3, + workerPath: '/tmp/foo', + }); + + worker.forceExit(); + forkInterface.emit('exit', 143 /* SIGTERM exit code */); + await Promise.resolve(); + + jest.runAllTimers(); + expect(forkInterface.kill.mock.calls).toEqual([['SIGTERM']]); +}); diff --git a/packages/jest-worker/src/workers/__tests__/processChild.test.js b/packages/jest-worker/src/workers/__tests__/processChild.test.js index 521587bd9e0b..a369777625f0 100644 --- a/packages/jest-worker/src/workers/__tests__/processChild.test.js +++ b/packages/jest-worker/src/workers/__tests__/processChild.test.js @@ -18,7 +18,6 @@ import { CHILD_MESSAGE_INITIALIZE, CHILD_MESSAGE_CALL, CHILD_MESSAGE_END, - CHILD_MESSAGE_FORCE_EXIT, PARENT_MESSAGE_OK, PARENT_MESSAGE_CLIENT_ERROR, } from '../../types'; @@ -328,21 +327,6 @@ it('removes the message listener on END message', () => { expect(process.listenerCount('message')).toBe(0); }); -it('finishes the process with exit code 0 if requested', () => { - process.emit('message', [ - CHILD_MESSAGE_INITIALIZE, - true, // Not really used here, but for flow type purity. - './my-fancy-worker', - ]); - - process.emit('message', [ - CHILD_MESSAGE_FORCE_EXIT, - true, // Not really used here, but for flow type purity. - ]); - - expect(process.exit.mock.calls[0]).toEqual([0]); -}); - it('calls the teardown method ', () => { process.emit('message', [ CHILD_MESSAGE_INITIALIZE, diff --git a/packages/jest-worker/src/workers/processChild.ts b/packages/jest-worker/src/workers/processChild.ts index 46b645bef4a1..102466d6d8d9 100644 --- a/packages/jest-worker/src/workers/processChild.ts +++ b/packages/jest-worker/src/workers/processChild.ts @@ -8,7 +8,6 @@ import { CHILD_MESSAGE_CALL, CHILD_MESSAGE_END, - CHILD_MESSAGE_FORCE_EXIT, CHILD_MESSAGE_INITIALIZE, PARENT_MESSAGE_CLIENT_ERROR, PARENT_MESSAGE_ERROR, @@ -52,10 +51,6 @@ const messageListener = (request: any) => { end(); break; - case CHILD_MESSAGE_FORCE_EXIT: - forceExit(); - break; - default: throw new TypeError( 'Unexpected request from parent process: ' + request[0], @@ -116,10 +111,6 @@ function exitProcess(): void { process.removeListener('message', messageListener); } -function forceExit(): void { - process.exit(0); -} - function execMethod(method: string, args: Array): void { const main = require(file!);