From 4604dffadb4414e31e2bc38e297102fdb33e90b9 Mon Sep 17 00:00:00 2001 From: Brandon Cheng Date: Sat, 29 Oct 2022 00:25:58 -0400 Subject: [PATCH] fix(jest-worker): fix hanging when child process workers are killed --- CHANGELOG.md | 1 + .../src/workers/ChildProcessWorker.ts | 42 +++++++++++- .../workers/__tests__/WorkerEdgeCases.test.ts | 65 +++++++++++++++++++ .../__tests__/__fixtures__/SelfKillWorker.js | 24 +++++++ 4 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 packages/jest-worker/src/workers/__tests__/__fixtures__/SelfKillWorker.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 494e36427afb..e90b4a88d923 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - `[jest-mock]` Treat cjs modules as objects so they can be mocked ([#13513](https://github.com/facebook/jest/pull/13513)) +- `[jest-worker]` Throw an error instead of hanging when jest workers are killed ([#13565](https://github.com/facebook/jest/pull/13565)) ### Chore & Maintenance diff --git a/packages/jest-worker/src/workers/ChildProcessWorker.ts b/packages/jest-worker/src/workers/ChildProcessWorker.ts index e3999fa49e1a..691bbb6edefd 100644 --- a/packages/jest-worker/src/workers/ChildProcessWorker.ts +++ b/packages/jest-worker/src/workers/ChildProcessWorker.ts @@ -343,7 +343,7 @@ export default class ChildProcessWorker } } - private _onExit(exitCode: number | null) { + private _onExit(exitCode: number | null, signal: NodeJS.Signals | null) { this._workerReadyPromise = undefined; this._resolveWorkerReady = undefined; @@ -372,6 +372,46 @@ export default class ChildProcessWorker this._child.send(this._request); } } else { + // At this point, it's not clear why the child process exited. There could + // be several reasons: + // + // 1. The child process exited successfully after finishing its work. + // This is the most likely case. + // 2. The child process crashed in a manner that wasn't caught through + // any of the heuristic-based checks above. + // 3. The child process was killed by another process or daemon unrelated + // to Jest. For example, oom-killer on Linux may have picked the child + // process to kill because overall system memory is constrained. + // + // If there's a pending request to the child process in any of those + // situations, the request still needs to be handled in some manner before + // entering the shutdown phase. Otherwise the caller expecting a response + // from the worker will never receive indication that something unexpected + // happened and hang forever. + // + // In normal operation, the request is handled and cleared before the + // child process exits. If it's still present, it's not clear what + // happened and probably best to throw an error. In practice, this usually + // happens when the child process is killed externally. + // + // There's a reasonable argument that the child process should be retried + // with request re-sent in this scenario. However, if the problem was due + // to situations such as oom-killer attempting to free up system + // resources, retrying would exacerbate the problem. + const isRequestStillPending = !!this._request; + if (isRequestStillPending) { + // If a signal is present, we can be reasonably confident the process + // was killed externally. Log this fact so it's more clear to users that + // something went wrong externally, rather than a bug in Jest itself. + const error = new Error( + signal != null + ? `A jest worker process (pid=${this._child.pid}) was terminated by another process: signal=${signal}, exitCode=${exitCode}. Operating system logs may contain more information on why this occurred.` + : `A jest worker process (pid=${this._child.pid}) crashed for an unknown reason: exitCode=${exitCode}`, + ); + + this._onProcessEnd(error, null); + } + this._shutdown(); } } diff --git a/packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts b/packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts index e056531854ae..9fbb84dd60d8 100644 --- a/packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts +++ b/packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts @@ -386,3 +386,68 @@ describe.each([ }); }); }); + +// This describe block only applies to the child process worker since it's +// generally not possible for external processes to abruptly kill a thread of +// another process. +describe('should not hang on external process kill', () => { + let worker: ChildProcessWorker; + + beforeEach(() => { + const options = { + childWorkerPath: processChildWorkerPath, + maxRetries: 0, + silent: true, + workerPath: join(__dirname, '__fixtures__', 'SelfKillWorker'), + } as unknown as WorkerOptions; + + worker = new ChildProcessWorker(options); + }); + + afterEach(async () => { + await new Promise(resolve => { + setTimeout(async () => { + if (worker) { + worker.forceExit(); + await worker.waitForExit(); + } + + resolve(); + }, 500); + }); + }); + + // Regression test for https://github.com/facebook/jest/issues/13183 + test('onEnd callback is called', async () => { + let onEndPromiseResolve: () => void; + let onEndPromiseReject: (err: Error) => void; + const onEndPromise = new Promise((resolve, reject) => { + onEndPromiseResolve = resolve; + onEndPromiseReject = reject; + }); + + const onStart = jest.fn(); + const onEnd = jest.fn((err: Error | null) => { + if (err) { + return onEndPromiseReject(err); + } + onEndPromiseResolve(); + }); + const onCustom = jest.fn(); + + await worker.waitForWorkerReady(); + + // The SelfKillWorker simulates an external process calling SIGTERM on it, + // but just SIGTERMs itself underneath the hood to make this test easier. + worker.send( + [CHILD_MESSAGE_CALL, true, 'selfKill', []], + onStart, + onEnd, + onCustom, + ); + + // The onEnd callback should be called when the child process exits. + await expect(onEndPromise).rejects.toBeInstanceOf(Error); + expect(onEnd).toHaveBeenCalled(); + }); +}); diff --git a/packages/jest-worker/src/workers/__tests__/__fixtures__/SelfKillWorker.js b/packages/jest-worker/src/workers/__tests__/__fixtures__/SelfKillWorker.js new file mode 100644 index 000000000000..49e841771486 --- /dev/null +++ b/packages/jest-worker/src/workers/__tests__/__fixtures__/SelfKillWorker.js @@ -0,0 +1,24 @@ +/** + * 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. + */ +const {isMainThread} = require('worker_threads'); + +async function selfKill() { + // This test is intended for the child process worker. If the Node.js worker + // thread mode is accidentally tested instead, let's prevent a confusing + // situation where process.kill stops the Jest test harness itself. + if (!isMainThread) { + // process.exit is documented to only stop the current thread rather than + // the process in a worker_threads environment. + process.exit(); + } + + process.kill(process.pid); +} + +module.exports = { + selfKill, +};