Skip to content

Commit 195cb4b

Browse files
authored
fix(jest-worker): fix hanging when workers are killed or unexpectedly exit (#13566)
1 parent 6483979 commit 195cb4b

File tree

5 files changed

+137
-1
lines changed

5 files changed

+137
-1
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
### Fixes
88

99
- `[jest-mock]` Treat cjs modules as objects so they can be mocked ([#13513](https://github.com/facebook/jest/pull/13513))
10+
- `[jest-worker]` Throw an error instead of hanging when jest workers terminate unexpectedly ([#13566](https://github.com/facebook/jest/pull/13566))
1011

1112
### Chore & Maintenance
1213

packages/jest-worker/src/workers/ChildProcessWorker.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ export default class ChildProcessWorker
343343
}
344344
}
345345

346-
private _onExit(exitCode: number | null) {
346+
private _onExit(exitCode: number | null, signal: NodeJS.Signals | null) {
347347
this._workerReadyPromise = undefined;
348348
this._resolveWorkerReady = undefined;
349349

@@ -372,6 +372,46 @@ export default class ChildProcessWorker
372372
this._child.send(this._request);
373373
}
374374
} else {
375+
// At this point, it's not clear why the child process exited. There could
376+
// be several reasons:
377+
//
378+
// 1. The child process exited successfully after finishing its work.
379+
// This is the most likely case.
380+
// 2. The child process crashed in a manner that wasn't caught through
381+
// any of the heuristic-based checks above.
382+
// 3. The child process was killed by another process or daemon unrelated
383+
// to Jest. For example, oom-killer on Linux may have picked the child
384+
// process to kill because overall system memory is constrained.
385+
//
386+
// If there's a pending request to the child process in any of those
387+
// situations, the request still needs to be handled in some manner before
388+
// entering the shutdown phase. Otherwise the caller expecting a response
389+
// from the worker will never receive indication that something unexpected
390+
// happened and hang forever.
391+
//
392+
// In normal operation, the request is handled and cleared before the
393+
// child process exits. If it's still present, it's not clear what
394+
// happened and probably best to throw an error. In practice, this usually
395+
// happens when the child process is killed externally.
396+
//
397+
// There's a reasonable argument that the child process should be retried
398+
// with request re-sent in this scenario. However, if the problem was due
399+
// to situations such as oom-killer attempting to free up system
400+
// resources, retrying would exacerbate the problem.
401+
const isRequestStillPending = !!this._request;
402+
if (isRequestStillPending) {
403+
// If a signal is present, we can be reasonably confident the process
404+
// was killed externally. Log this fact so it's more clear to users that
405+
// something went wrong externally, rather than a bug in Jest itself.
406+
const error = new Error(
407+
signal != null
408+
? `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.`
409+
: `A jest worker process (pid=${this._child.pid}) crashed for an unknown reason: exitCode=${exitCode}`,
410+
);
411+
412+
this._onProcessEnd(error, null);
413+
}
414+
375415
this._shutdown();
376416
}
377417
}

packages/jest-worker/src/workers/NodeThreadsWorker.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,18 @@ export default class ExperimentalWorker
254254
this._worker.postMessage(this._request);
255255
}
256256
} else {
257+
// If the worker thread exits while a request is still pending, throw an
258+
// error. This is unexpected and tests may not have run to completion.
259+
const isRequestStillPending = !!this._request;
260+
if (isRequestStillPending) {
261+
this._onProcessEnd(
262+
new Error(
263+
'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.',
264+
),
265+
null,
266+
);
267+
}
268+
257269
this._shutdown();
258270
}
259271
}

packages/jest-worker/src/workers/__tests__/WorkerEdgeCases.test.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,4 +385,66 @@ describe.each([
385385
);
386386
});
387387
});
388+
389+
describe('should not hang when worker is killed or unexpectedly terminated', () => {
390+
let worker: ChildProcessWorker | ThreadsWorker;
391+
392+
beforeEach(() => {
393+
const options = {
394+
childWorkerPath: processChildWorkerPath,
395+
maxRetries: 0,
396+
silent: true,
397+
workerPath: join(__dirname, '__fixtures__', 'SelfKillWorker'),
398+
} as unknown as WorkerOptions;
399+
400+
worker = new ChildProcessWorker(options);
401+
});
402+
403+
afterEach(async () => {
404+
await new Promise<void>(resolve => {
405+
setTimeout(async () => {
406+
if (worker) {
407+
worker.forceExit();
408+
await worker.waitForExit();
409+
}
410+
411+
resolve();
412+
}, 500);
413+
});
414+
});
415+
416+
// Regression test for https://github.com/facebook/jest/issues/13183
417+
test('onEnd callback is called', async () => {
418+
let onEndPromiseResolve: () => void;
419+
let onEndPromiseReject: (err: Error) => void;
420+
const onEndPromise = new Promise<void>((resolve, reject) => {
421+
onEndPromiseResolve = resolve;
422+
onEndPromiseReject = reject;
423+
});
424+
425+
const onStart = jest.fn();
426+
const onEnd = jest.fn((err: Error | null) => {
427+
if (err) {
428+
return onEndPromiseReject(err);
429+
}
430+
onEndPromiseResolve();
431+
});
432+
const onCustom = jest.fn();
433+
434+
await worker.waitForWorkerReady();
435+
436+
// The SelfKillWorker simulates an external process calling SIGTERM on it,
437+
// but just SIGTERMs itself underneath the hood to make this test easier.
438+
worker.send(
439+
[CHILD_MESSAGE_CALL, true, 'selfKill', []],
440+
onStart,
441+
onEnd,
442+
onCustom,
443+
);
444+
445+
// The onEnd callback should be called when the child process exits.
446+
await expect(onEndPromise).rejects.toBeInstanceOf(Error);
447+
expect(onEnd).toHaveBeenCalled();
448+
});
449+
});
388450
});
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
const {isMainThread} = require('worker_threads');
8+
9+
async function selfKill() {
10+
if (!isMainThread) {
11+
// process.exit is documented to only stop the current thread rather than
12+
// the entire process in a worker_threads environment.
13+
process.exit();
14+
}
15+
16+
process.kill(process.pid);
17+
}
18+
19+
module.exports = {
20+
selfKill,
21+
};

0 commit comments

Comments
 (0)