Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(jest-worker): failed assertion with circular value hangs the test run #14675

Closed
wants to merge 8 commits into from
Closed
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
- [**BREAKING**] Changes `testPathPattern` configuration option to `testPathPatterns`, which now takes a list of patterns instead of the regex.
- [**BREAKING**] `--testPathPattern` is now `--testPathPatterns`
- `[jest-reporters, jest-runner]` Unhandled errors without stack get correctly logged to console ([#14619](https://github.com/facebook/jest/pull/14619))
- [jest-worker] Properly handle a circular reference error when worker tries to send an assertion fails where either the expected or actual value is circular ([#14675](https://github.com/jestjs/jest/pull/14675))

### Performance

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import ThreadsWorker from '../NodeThreadsWorker';
jest.setTimeout(10_000);

const root = join('../../');
const filesToBuild = ['workers/processChild', 'workers/threadChild', 'types'];
const filesToBuild = [
'workers/processChild',
'workers/threadChild',
'workers/withoutCircularRefs',
'types',
];
const writeDestination = join(__dirname, '__temp__');
const processChildWorkerPath = join(
writeDestination,
Expand Down
33 changes: 33 additions & 0 deletions packages/jest-worker/src/workers/__tests__/processChild.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ beforeEach(() => {
mockCount++;

return {
fooCircularResult() {
const circular = {self: undefined as unknown};
circular.self = circular;
return {error: circular};
},

fooPromiseThrows() {
return new Promise((_resolve, reject) => {
setTimeout(() => reject(mockError), 5);
Expand Down Expand Up @@ -338,6 +344,33 @@ it('returns results when it gets resolved if function is asynchronous', async ()
expect(spyProcessSend).toHaveBeenCalledTimes(2);
});

it('returns results with circular references', () => {
process.emit(
'message',
[
CHILD_MESSAGE_INITIALIZE,
true, // Not really used here, but for type purity.
'./my-fancy-worker',
],
null,
);

process.emit(
'message',
[
CHILD_MESSAGE_CALL,
true, // Not really used here, but for type purity.
'fooCircularResult',
[],
],
null,
);

expect(spyProcessSend.mock.calls[0][0][1]).toEqual({
error: {self: expect.anything()},
});
});

it('calls the main module if the method call is "default"', () => {
process.emit(
'message',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {withoutCircularRefs} from '../withoutCircularRefs';

it('test simple values', () => {
expect(withoutCircularRefs(undefined)).toBeUndefined();
expect(withoutCircularRefs(null)).toBeNull();
expect(withoutCircularRefs(0)).toBe(0);
expect(withoutCircularRefs('12')).toBe('12');
expect(withoutCircularRefs(true)).toBe(true);
expect(withoutCircularRefs([1])).toEqual([1]);
expect(withoutCircularRefs({a: 1, b: {c: 2}})).toEqual({a: 1, b: {c: 2}});
});

it('test circular values', () => {
const circular = {self: undefined as any};
circular.self = circular;

expect(withoutCircularRefs(circular)).toEqual({self: '[Circular]'});

expect(withoutCircularRefs([{a: circular, b: null}])).toEqual([
{a: {self: '[Circular]'}, b: null},
]);

expect(withoutCircularRefs({a: {b: circular}, c: undefined})).toEqual({
a: {b: {self: '[Circular]'}, c: undefined},
});
});
17 changes: 16 additions & 1 deletion packages/jest-worker/src/workers/messageParent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import {isMainThread, parentPort} from 'worker_threads';
import {PARENT_MESSAGE_CUSTOM} from '../types';
import {withoutCircularRefs} from './withoutCircularRefs';

export default function messageParent(
message: unknown,
Expand All @@ -15,7 +16,21 @@ export default function messageParent(
if (!isMainThread && parentPort != null) {
parentPort.postMessage([PARENT_MESSAGE_CUSTOM, message]);
} else if (typeof parentProcess.send === 'function') {
parentProcess.send([PARENT_MESSAGE_CUSTOM, message]);
try {
parentProcess.send([PARENT_MESSAGE_CUSTOM, message]);
} catch (e: any) {
if (/circular structure/.test(e?.message)) {
// We can safely send a message to the parent process again
// because previous sending was halted by "TypeError: Converting circular structure to JSON".
// But this time the message will be cleared from circular references.
parentProcess.send([
PARENT_MESSAGE_CUSTOM,
withoutCircularRefs(message),
]);
} else {
throw e;
}
nodkz marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
throw new Error('"messageParent" can only be used inside a worker');
}
Expand Down
14 changes: 13 additions & 1 deletion packages/jest-worker/src/workers/processChild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
PARENT_MESSAGE_SETUP_ERROR,
type ParentMessageMemUsage,
} from '../types';
import {withoutCircularRefs} from './withoutCircularRefs';

type UnknownFunction = (...args: Array<unknown>) => unknown | Promise<unknown>;

Expand Down Expand Up @@ -97,7 +98,18 @@ function reportSuccess(result: unknown) {
throw new Error('Child can only be used on a forked process');
}

process.send([PARENT_MESSAGE_OK, result]);
try {
process.send([PARENT_MESSAGE_OK, result]);
} catch (e: any) {
// We can safely send a message to the parent process again
// because previous sending was halted by "TypeError: Converting circular structure to JSON".
// But this time the message will be cleared from circular references.
if (e && /circular structure/.test(e?.message)) {
process.send([PARENT_MESSAGE_OK, withoutCircularRefs(result)]);
} else {
throw e;
}
nodkz marked this conversation as resolved.
Show resolved Hide resolved
}
}

function reportClientError(error: Error) {
Expand Down
27 changes: 27 additions & 0 deletions packages/jest-worker/src/workers/withoutCircularRefs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

export function withoutCircularRefs(obj: unknown): unknown {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do the inverse as well - rebuild the object with the circular references in place?

I.e. what https://www.npmjs.com/package/flatted does (I see they now wanna push users to https://github.com/ungap/structured-clone/blob/main/README.md#extra-features, might be worth a look)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB flatted and structured-clone have own serialization format, so both jest-runner and jest-worker should have the same serialize/parse logic. But currently runner and worker are different packages and end-users might have different versions on their installation.

So we break a transport layer by this improvement. Better to do it in another PR as improvement and R&D, this PR is a bug fix PR. And I'm very sorry but I don't have enough bandwidth to deal with bumping v30 😉

const cache = new WeakSet();
function copy(obj: unknown) {
if (typeof obj !== 'object' || obj === null) {
return obj;
}
if (cache.has(obj)) {
return '[Circular]';
}
cache.add(obj);
const copyObj: any = Array.isArray(obj) ? [] : {};
for (const key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key)) {
copyObj[key] = copy((obj as any)[key]);
}
}
return copyObj;
}
return copy(obj);
}