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): Remove circular references from messages sent to workers #10881

Closed
wants to merge 1 commit into from

Conversation

ziacik
Copy link

@ziacik ziacik commented Nov 27, 2020

Fixes #10577

Summary

This fix prevents objects with circular references to be sent to worker processes. Node uses JSON.stringify to serialize such messages and this causes an error in case there are circular references in the messages.

Please note I added new dependency, fclone, though I'm not sure it's allowed or if there already is another such function in the repository.

Test plan

This fix allows to run the tests described in the issue linked. The repro setup can be found at https://repl.it/@Frantiekiaik/jest-playground-1.

@ziacik ziacik force-pushed the runner-circular-references branch 2 times, most recently from d67338b to 75e522f Compare November 27, 2020 20:55
@SimenB
Copy link
Member

SimenB commented Nov 28, 2020

We'd want to restore the original object on the receiving side, no? Should we use something like https://github.com/storybookjs/telejson?

@ziacik
Copy link
Author

ziacik commented Nov 28, 2020

@SimenB sounds reasonable. I'll try to rework.

@MichaReiser
Copy link
Contributor

MichaReiser commented Dec 6, 2020

Would the 'advanced' serialization format be an alternative for use-cases that require circular references to avoid that all workers must pay for the overhead of fclone? This would also solve the issue where e.g. a child returns a structure containing circular references.

I would suggest to not enable this option by default because it comes with a slight performance overhead.

@SimenB
Copy link
Member

SimenB commented Dec 28, 2020

@MichaReiser I tried that, but it just changes the error to the same as worker threads

(node:24057) UnhandledPromiseRejectionWarning: Error: () =>
          (0, _jestMatcherUtils.matcherHint)(
            matcherName,
            undefined,
           ...<omitted>... ) could not be cloned.
    at writeChannelMessage (internal/child_process/serialization.js:79:9)
    at process.target._send (internal/child_process.js:805:17)
    at process.target.send (internal/child_process.js:703:19)
    at reportSuccess (/Users/simen/repos/jest/packages/jest-worker/build/workers/processChild.js:67:11)

While using worker (which uses structured clone by default)

(node:24148) UnhandledPromiseRejectionWarning: DataCloneError: () =>
          (0, _jestMatcherUtils.matcherHint)(
            matcherName,
            undefined,
           ...<omitted>... ) could not be cloned.
    at reportSuccess (/Users/simen/repos/jest/packages/jest-worker/build/workers/threadChild.js:77:32)

@MichaReiser
Copy link
Contributor

MichaReiser commented Dec 28, 2020

@SimenB that's rather interesting because the MDN documentation explicitly mentions circular references...

The structured clone algorithm copies complex JavaScript objects. It is used internally to transfer data between Workers via postMessage(), storing objects with IndexedDB, or copying objects for other APIs. It clones by recursing through the input object while maintaining a map of previously visited references, to avoid infinitely traversing cycles. source.

What kind of object did you pass? Any chance it did use Proxies, contained Symbols, or used other features that make it not a plain JS object? spec

However, it is as it is. I still believe that it would make sense to change jest worker to allow passing in a custom serializer / deserialiser instead of requiring a custom serialization logic for all clients. Or manually calling fclone before invoking the method on the jest-worker / reading the value in the worker. E.g. I personally would prefer to get an error than being surprised by the fact that some properties in the worker/ in the result have been changed to [Circular] for no obvious reasons.

@SimenB
Copy link
Member

SimenB commented Dec 28, 2020

I've opened up #10981 which uses telejson and should restore the object "on the other side". I haven't dug into why passing circular objects doesn't work. Might be a good idea to do so as there's probably less overhead in fixing the object we pass (if possible) rather than adding custom serialization and deserialization

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
@ziacik ziacik deleted the runner-circular-references branch June 25, 2021 13:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circular references hang jest when assertions fail on node 14
4 participants