From a0703c94936e2459ae8600a30895da6f6b8b05de Mon Sep 17 00:00:00 2001 From: Brandon Cheng Date: Sat, 29 Oct 2022 00:25:58 -0400 Subject: [PATCH 1/2] 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..7ede48932a98 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 ([#13566](https://github.com/facebook/jest/pull/13566)) ### 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, +}; From f274baf4e06db1de48840853dc678d33942f1bab Mon Sep 17 00:00:00 2001 From: Brandon Cheng Date: Sun, 6 Nov 2022 15:03:33 -0500 Subject: [PATCH 2/2] fix(jest-worker): fix hanging after process.exit() in thread workers --- CHANGELOG.md | 2 +- .../src/workers/NodeThreadsWorker.ts | 12 ++ .../workers/__tests__/WorkerEdgeCases.test.ts | 105 +++++++++--------- .../__tests__/__fixtures__/SelfKillWorker.js | 5 +- 4 files changed, 65 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ede48932a98..30a210af1bc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +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 ([#13566](https://github.com/facebook/jest/pull/13566)) +- `[jest-worker]` Throw an error instead of hanging when jest workers terminate unexpectedly ([#13566](https://github.com/facebook/jest/pull/13566)) ### Chore & Maintenance diff --git a/packages/jest-worker/src/workers/NodeThreadsWorker.ts b/packages/jest-worker/src/workers/NodeThreadsWorker.ts index c6c0584c6ec2..2acf0054ae5d 100644 --- a/packages/jest-worker/src/workers/NodeThreadsWorker.ts +++ b/packages/jest-worker/src/workers/NodeThreadsWorker.ts @@ -254,6 +254,18 @@ export default class ExperimentalWorker this._worker.postMessage(this._request); } } else { + // If the worker thread exits while a request is still pending, throw an + // error. This is unexpected and tests may not have run to completion. + const isRequestStillPending = !!this._request; + if (isRequestStillPending) { + this._onProcessEnd( + new Error( + 'A Jest worker thread exited unexpectedly before finishing tests for an unknown reason. One of the ways this can happen is if process.exit() was called in testing code.', + ), + 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 9fbb84dd60d8..261cdb6b3d29 100644 --- a/packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts +++ b/packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts @@ -385,69 +385,66 @@ 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); - }); + describe('should not hang when worker is killed or unexpectedly terminated', () => { + let worker: ChildProcessWorker | ThreadsWorker; - afterEach(async () => { - await new Promise(resolve => { - setTimeout(async () => { - if (worker) { - worker.forceExit(); - await worker.waitForExit(); - } + beforeEach(() => { + const options = { + childWorkerPath: processChildWorkerPath, + maxRetries: 0, + silent: true, + workerPath: join(__dirname, '__fixtures__', 'SelfKillWorker'), + } as unknown as WorkerOptions; - resolve(); - }, 500); + worker = new ChildProcessWorker(options); }); - }); - // 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; - }); + afterEach(async () => { + await new Promise(resolve => { + setTimeout(async () => { + if (worker) { + worker.forceExit(); + await worker.waitForExit(); + } - const onStart = jest.fn(); - const onEnd = jest.fn((err: Error | null) => { - if (err) { - return onEndPromiseReject(err); - } - onEndPromiseResolve(); + resolve(); + }, 500); + }); }); - const onCustom = jest.fn(); - await worker.waitForWorkerReady(); + // 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; + }); - // 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, - ); + const onStart = jest.fn(); + const onEnd = jest.fn((err: Error | null) => { + if (err) { + return onEndPromiseReject(err); + } + onEndPromiseResolve(); + }); + const onCustom = jest.fn(); - // The onEnd callback should be called when the child process exits. - await expect(onEndPromise).rejects.toBeInstanceOf(Error); - expect(onEnd).toHaveBeenCalled(); + 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 index 49e841771486..9f39dfbfcdbf 100644 --- a/packages/jest-worker/src/workers/__tests__/__fixtures__/SelfKillWorker.js +++ b/packages/jest-worker/src/workers/__tests__/__fixtures__/SelfKillWorker.js @@ -7,12 +7,9 @@ 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. + // the entire process in a worker_threads environment. process.exit(); }