Skip to content

child_process: queue pending messages#41221

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.

Sign up for GitHub

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

Jump to bottom
Merged
Trott merged 1 commit into from
Dec 30, 2021
Merged

child_process: queue pending messages #41221

Trott merged 1 commit into from
Dec 30, 2021
+49 −1

Conversation

@ErickWendel
Copy link
Member

@ErickWendelErickWendel commented Dec 17, 2021

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

@nodejs-github-botnodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Dec 17, 2021
@ErickWendelErickWendelforce-pushed the erickwendel/queue-pending-messages-fork-process branch 2 times, most recently from 3b94fc1 to eb8c12aCompareDecember 17, 2021 15:21
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
@ErickWendelErickWendelforce-pushed the erickwendel/queue-pending-messages-fork-process branch 3 times, most recently from da5b44c to eef3572CompareDecember 17, 2021 15:50
@ErickWendelErickWendel deleted the erickwendel/queue-pending-messages-fork-process branch December 17, 2021 15:55
@ErickWendelErickWendel restored the erickwendel/queue-pending-messages-fork-process branch December 17, 2021 15:56
edsadr
edsadr requested changes Dec 17, 2021
View reviewed changes
Copy link
Member

@edsadredsadr left a 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

aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
aduh95
aduh95 reviewed Dec 17, 2021
View reviewed changes
@ErickWendelErickWendelforce-pushed the erickwendel/queue-pending-messages-fork-process branch from eef3572 to 49abb89CompareDecember 17, 2021 18:20
@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2021

Could you add a test please? Let me know if you need help writing one.

@ErickWendel
Copy link
MemberAuthor

ErickWendel commented Dec 17, 2021•
edited
Loading

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?

@aduh95
Copy link
Contributor

aduh95 commented Dec 17, 2021

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.

@ErickWendelErickWendelforce-pushed the erickwendel/queue-pending-messages-fork-process branch from 49abb89 to 0f48517CompareDecember 17, 2021 19:07
@ErickWendel
Copy link
MemberAuthor

ErickWendel commented Dec 17, 2021

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.

Got it! Let me know if this is what you suggested.

@ErickWendelErickWendelforce-pushed the erickwendel/queue-pending-messages-fork-process branch 3 times, most recently from 0168b1b to 100b785CompareDecember 17, 2021 19:11
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@ErickWendel@danielleadams
child_process: queue pending messages
00283c9
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]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@ErickWendel@Linkgoron
child_process: queue pending messages
129108a
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]>
@merceyzmerceyz mentioned this pull request Jan 31, 2022
Merged
3 tasks
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@ErickWendel@danielleadams
child_process: queue pending messages
9086338
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]>
@danielleadamsdanielleadams mentioned this pull request Feb 1, 2022
Merged
mateus-f-torres added a commit to mateus-f-torres/barefoot that referenced this pull request Feb 7, 2022
@mateus-f-torres
chore(node): upgrade to v17.4.0
d5644b1
Needed to resolve issue with Jest process (nodejs/node#41221)
@merceyzmerceyz mentioned this pull request Feb 8, 2022
Merged
3 tasks
mateus-f-torres added a commit to mateus-f-torres/barefoot that referenced this pull request Feb 9, 2022
@mateus-f-torres
chore(node): upgrade to v17.4.0
cfec40c
Needed to resolve issue with Jest process (nodejs/node#41221)
@merceyzmerceyz mentioned this pull request Feb 17, 2022
Closed
1 task
@ijjkijjk mentioned this pull request Feb 22, 2022
Merged
@ErickWendelErickWendel mentioned this pull request Mar 3, 2022
Closed
mateus-f-torres added a commit to mateus-f-torres/barefoot that referenced this pull request Mar 24, 2022
@mateus-f-torres
chore(node): upgrade to v17.4.0
8c83c06
Needed to resolve issue with Jest process (nodejs/node#41221)
mateus-f-torres added a commit to mateus-f-torres/barefoot that referenced this pull request Mar 24, 2022
@mateus-f-torres
chore(node): upgrade to v17.4.0
a42c15b
Needed to resolve issue with Jest process (nodejs/node#41221)
@merceyzmerceyz mentioned this pull request Apr 13, 2022
Closed
1 task
@sublimator
Copy link

sublimator commented Apr 22, 2022

Will this be fixed in 14 LTS ?

@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2022

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.

@ErickWendelErickWendel mentioned this pull request Apr 23, 2022
Closed
@ErickWendel
Copy link
MemberAuthor

ErickWendel commented Apr 23, 2022

@aduh95@sublimator just added a PR for backing porting this feature for 14.x. 🤩

@aduh95aduh95 added backport-open-v14.x and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 23, 2022
@juanarbol
Copy link
Member

juanarbol commented May 1, 2022

Will this be fixed in 14 LTS ?

It will ❤️

juanarbol pushed a commit that referenced this pull request May 1, 2022
@ErickWendel@juanarbol
child_process: queue pending messages
4203d13
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
@juanarboljuanarbol mentioned this pull request May 1, 2022
Merged
tujoworker added a commit to dnbexperience/eufemia that referenced this pull request Jun 13, 2022
@tujoworker
chore(repo): update Node.js version to latest v14
da146d4
This version includes a fix so we can/would run jest with Yarn PnP: nodejs/node#41221
@tujoworkertujoworker mentioned this pull request Jun 13, 2022
Closed
@merceyzmerceyz mentioned this pull request Jan 30, 2023
Closed
@aboe026aboe026 mentioned this pull request Jan 31, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments

@addaleaxaddaleaxaddaleax left review comments

@aduh95aduh95aduh95 approved these changes

@edsadredsadredsadr approved these changes

@JungMinuJungMinuJungMinu approved these changes

+5 more reviewers

@ceoofyourmotherceoofyourmotherceoofyourmother left review comments

@felipe-ssilvafelipe-ssilvafelipe-ssilva approved these changes

@IcaroBichirIcaroBichirIcaroBichir approved these changes

@brenofsenabrenofsenabrenofsena approved these changes

@joaopcmjoaopcmjoaopcm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.child_processIssues and PRs related to the child_process subsystem.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

discussion about how to fix child_process 'spawn' not ready to send messages on ESM

18 participants

@ErickWendel@aduh95@nodejs-github-bot@Trott@Peeja@sublimator@juanarbol@jasnell@addaleax@edsadr@felipe-ssilva@JungMinu@IcaroBichir@brenofsena@joaopcm@ceoofyourmother@danielleadams@DerekNonGeneric