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: ignore all unrelated messages from child process #13543

Merged
merged 3 commits into from
Feb 3, 2023

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Oct 31, 2022

Summary

This is a small part of #13521 that I felt would be easier to review and merge on its own. It makes sure that any other part of the code that also uses IPC (e.g. a Node.js loader) doesn't interfere and crash the Jest process.

Test plan

I've tested this by running yarn build and manually coping the changed files into another small test project 🙈

@LinusU
Copy link
Contributor Author

LinusU commented Nov 7, 2022

rebased on master

ping @SimenB @privatenumber, is there anything that I can do in order to move this forward?

@SimenB
Copy link
Member

SimenB commented Nov 7, 2022

rebased on master

ping @SimenB @privatenumber, is there anything that I can do in order to move this forward?

Sorry, just swamped at work atm 🙂


We should mirror this functionality in the worker_threads implementation

@LinusU
Copy link
Contributor Author

LinusU commented Nov 8, 2022

Sorry, just swamped at work atm 🙂

No worries at all!

We should mirror this functionality in the worker_threads implementation

Will do 👍

@LinusU
Copy link
Contributor Author

LinusU commented Nov 8, 2022

Rebased on latest main, and mirrored the changes for the worker_threads implementation 🐎

@jeysal
Copy link
Contributor

jeysal commented Nov 8, 2022

I don't have a whole lot of context on the whole ESM project that @SimenB has almost entirely been handling, but being lenient on this seems dangerous to me. Unless every tool and library out there starts namespacing their IPC do be clearly distinguishable (almost like making it a standard protocol), there's risk of catching and misinterpreting others' messages. I think that the code that spawns the child (which is jest-worker) owning the IPC communication is reasonable. However, if there is some standard for namespacing Node IPC communication (I don't know), it could make sense for us to follow that starting from a next jest-worker major version, freeing up the rest of the channel. I'll go comment on the other issue as well

@privatenumber
Copy link

To prevent collision, jest should add some sort of integrity check.

For example, prefixing the message with the name and version properties from package.json would probably be unique enough. For something super secure, generating a UUID in the parent, spawning the worker with the UUID as an argument, and prefixing the messages with it.

@jeysal
Copy link
Contributor

jeysal commented Nov 8, 2022

Yes that would be a way of namespacing the IPC. I wouldn't be opposed to changing jest-worker to do this, but I'm still wondering if it's necessary because I haven't seen something like this in action anywhere. And it seems to me like a reasonable assumption that the code spawning a process gets to interact with it via IPC and other code that had nothing to do with spawning the child should not touch the IPC channel.

Like, can you point to examples of other ecosystem tools or libraries namespacing IPC in this way? Or to examples of libraries doing child-parent communication even though they were not responsible for spawning a child?

I'm just not sure that it's actually Jest using an unclean approach here, versus Jest keeping it simple as everyone does and esm-loader assuming access to an IPC that it does not own and should not touch.

Also cc @SimenB in case you know whether it is convention for libs to assume they can use the IPC channel

@privatenumber
Copy link

I don't think a convention is necessary to move forward.

esm-loader assuming access to an IPC that it does not own and should not touch.

It's not owned by jest either. The parent node process owns the IPC. In this case, jest and esm-loader are executed in the same node process so it's shared.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

works for me 👍

@SimenB SimenB merged commit 48ddbf4 into jestjs:main Feb 3, 2023
@LinusU LinusU deleted the ignore-ipc-messages branch February 7, 2023 07:07
@SimenB
Copy link
Member

SimenB commented Feb 7, 2023

https://github.com/facebook/jest/releases/tag/v29.4.2

@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 Mar 10, 2023
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.

5 participants