-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
child_process: queue pending messages #41221
child_process: queue pending messages #41221
Conversation
3b94fc1
to
eb8c12a
Compare
da5b44c
to
eef3572
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good but pending from CI, will monitor and remove this once it passes
eef3572
to
49abb89
Compare
Could you add a test please? Let me know if you need help writing one. |
There're tests for it before. The problem is that on ESM the behavior is different. As the coverage didn't change I didn't create a test for it. Do you think it's really necessary? |
Yes, we could simply copy the test we have and port it to ESM. It is necessary, otherwise another future change may break it again for ESM and we wouldn't be able to tell. |
49abb89
to
0f48517
Compare
Got it! Let me know if this is what you suggested. |
0168b1b
to
100b785
Compare
It fixes the problem of the child process not receiving messages. Fixes: nodejs#41134 PR-URL: nodejs#41221 Reviewed-By: Adrian Estrada <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
It fixes the problem of the child process not receiving messages. Fixes: #41134 PR-URL: #41221 Reviewed-By: Adrian Estrada <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Needed to resolve issue with Jest process (nodejs/node#41221)
Needed to resolve issue with Jest process (nodejs/node#41221)
Needed to resolve issue with Jest process (nodejs/node#41221)
Needed to resolve issue with Jest process (nodejs/node#41221)
Will this be fixed in 14 LTS ? |
v14.x is in maintenance mode, meaning that no change land there unless someone opens a PR to backport it. See https://github.com/nodejs/node/blob/master/doc/contributing/backporting-to-release-lines.md for more info. |
@aduh95 @sublimator just added a PR for backing porting this feature for 14.x. 🤩 |
It will ❤️ |
It fixes the problem of the child process not receiving messages. Fixes: #41134 PR-URL: #41221 Reviewed-By: Adrian Estrada <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Backport-PR-URL: #42840
This version includes a fix so we can/would run jest with Yarn PnP: nodejs/node#41221
It fixes the problem for child process not receiving messages when forking a process and immediately sending messages on ESM by queuing messages until the process is ready.
Fixes: #41134
Refs: #37782, #39140, #39140, #34785, and help/issues/1383