Skip to content

Commit

Permalink
process.exit => SIGTERM & SIGKILL
Browse files Browse the repository at this point in the history
  • Loading branch information
jeysal committed Mar 25, 2019
1 parent 47c8cea commit ae3f617
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 44 deletions.
11 changes: 6 additions & 5 deletions packages/jest-worker/src/base/BaseWorkerPool.ts
Expand Up @@ -10,16 +10,16 @@ import mergeStream from 'merge-stream';

import {
CHILD_MESSAGE_END,
CHILD_MESSAGE_FORCE_EXIT,
PoolExitResult,
WorkerPoolOptions,
WorkerOptions,
WorkerInterface,
} 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 = () => {};

Expand Down Expand Up @@ -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);

Expand All @@ -111,7 +111,8 @@ export default class BaseWorkerPool {
return forceExited;
});

return (await Promise.all(workerExitPromises)).reduce<PoolExitResult>(
const workerExits = await Promise.all(workerExitPromises);
return workerExits.reduce<PoolExitResult>(
(result, forceExited) => ({
forceExited: result.forceExited || forceExited,
}),
Expand Down
20 changes: 10 additions & 10 deletions packages/jest-worker/src/base/__tests__/BaseWorkerPool.test.js
Expand Up @@ -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';

Expand All @@ -28,6 +28,7 @@ describe('BaseWorkerPool', () => {
beforeEach(() => {
Worker.mockClear();
Worker.mockImplementation(() => ({
forceExit: jest.fn(),
getStderr: () => ({once() {}, pipe() {}}),
getStdout: () => ({once() {}, pipe() {}}),
send: jest.fn(),
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand All @@ -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();
});
});
});
4 changes: 1 addition & 3 deletions packages/jest-worker/src/types.ts
Expand Up @@ -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;
Expand Down Expand Up @@ -46,6 +45,7 @@ export interface WorkerInterface {
onProcessEnd: OnEnd,
): void;
waitForExit(): Promise<void>;
forceExit(): void;

getWorkerId(): number;
getStderr(): NodeJS.ReadableStream | null;
Expand Down Expand Up @@ -121,8 +121,6 @@ export type ChildMessageEnd = [
boolean // processed
];

export type ChildMessageForceExit = [typeof CHILD_MESSAGE_FORCE_EXIT];

export type ChildMessage =
| ChildMessageInitialize
| ChildMessageCall
Expand Down
17 changes: 16 additions & 1 deletion packages/jest-worker/src/workers/ChildProcessWorker.ts
Expand Up @@ -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:
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down
Expand Up @@ -19,6 +19,8 @@ import {
PARENT_MESSAGE_OK,
} from '../../types';

jest.useFakeTimers();

let Worker;
let forkInterface;
let childProcess;
Expand All @@ -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(),
Expand Down Expand Up @@ -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']]);
});
16 changes: 0 additions & 16 deletions packages/jest-worker/src/workers/__tests__/processChild.test.js
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 0 additions & 9 deletions packages/jest-worker/src/workers/processChild.ts
Expand Up @@ -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,
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -116,10 +111,6 @@ function exitProcess(): void {
process.removeListener('message', messageListener);
}

function forceExit(): void {
process.exit(0);
}

function execMethod(method: string, args: Array<any>): void {
const main = require(file!);

Expand Down

0 comments on commit ae3f617

Please sign in to comment.