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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ErickWendel opened this issue Dec 10, 2021 · 13 comments 路 Fixed by #41221
Closed

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

ErickWendel opened this issue Dec 10, 2021 · 13 comments 路 Fixed by #41221
Labels
child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@ErickWendel
Copy link
Member

ErickWendel commented Dec 10, 2021

BTW: I'd like to help fixing this bug 馃ぉ

Is your feature request related to a problem? Please describe.

There's a current issue on spawn's child processes using ES modules mentioned in those issues (#37782, #39140, #39140, #34785, and help/issues/1383 ).

When you fork a file and immediately use the .send event. The child process doesn't receive messages because it's not ready yet.

So we need to make a workaround. The child emits an event, the parent waits for .on('message', we check the message and then we start sending messages to the child.

Describe the solution you'd like
My plan initially was to add an event like child.on("ready to specify when we can start sending messages to the child process.

I spoke to @addaleax and she suggested a behavior that is closer to what MessagePorts expose, which is queueing up messages until a message listener is installed.

I know that it would increase implementation complexity a bit, but it would be very helpful for avoiding pitfalls like the ones linked here

What do you think? Any ideas of what could we do?

@ErickWendel ErickWendel added the feature request Issues that request new features to be added to Node.js. label Dec 10, 2021
@ErickWendel ErickWendel changed the title discussion about how to fix child_process 'spawn' event is emitted too soon on ESM discussion about how to fix child_process 'spawn' not ready to send messages on ESM Dec 10, 2021
@Mesteery Mesteery added the child_process Issues and PRs related to the child_process subsystem. label Dec 10, 2021
@Trott
Copy link
Member

Trott commented Dec 12, 2021

Seems like a fine idea to me but would want to hear what @nodejs/modules and/or @nodejs/child_process folks might have to say about it.

@bmeck
Copy link
Member

bmeck commented Dec 12, 2021

I'd prefer if we could queue things because this problem isn't unique to ESM and I've seen it in the long past using purely CJS and callbacks.

@ErickWendel
Copy link
Member Author

ErickWendel commented Dec 13, 2021

I'd prefer if we could queue things because this problem isn't unique to ESM and I've seen it in the long past using purely CJS and callbacks.

I think I didn't get it. What do you mean by queue things?

@gabrielsimas
Copy link

I'd prefer if we could queue things because this problem isn't unique to ESM and I've seen it in the long past using purely CJS and callbacks.

I don't get it too... do you can explain please?

@bmeck
Copy link
Member

bmeck commented Dec 13, 2021

@gabrielsimas MessagePort will queue messages (events) and wait for a listener before draining the messages. The problem in this issue is that event listeners which are used in child_process are not queueing messages and fire messages that the child process has loaded prior to the child process attaching any listeners to handle events.

@weritontmachado
Copy link

I spoke to @addaleax and she suggested a behavior that is closer to what MessagePorts expose, which is queueing up messages until a message listener is installed.

This was my first thought

@ErickWendel
Copy link
Member Author

@gabrielsimas MessagePort will queue messages (events) and wait for a listener before draining the messages. The problem in this issue is that event listeners which are used in child_process are not queueing messages and fire messages that the child process has loaded prior to the child process attaching any listeners to handle events.

niiice! It's the same strategy @addaleax suggested on the post, right?

@ktfth
Copy link

ktfth commented Dec 13, 2021

You have a reproduction of the behavior @ErickWendel?

@ktfth
Copy link

ktfth commented Dec 13, 2021

Or the behavior is the same on the related content you shared?

@ErickWendel
Copy link
Member Author

Or the behavior is the same on the related content you shared?

Yes, Actually they already had reproduced the behavior at all of the issues I mentioned in the post. See this one #39140

@mcollina
Copy link
Member

I'd prefer if we could queue things because this problem isn't unique to ESM and I've seen it in the long past using purely CJS and callbacks.

This is the way to go

ErickWendel added a commit to ErickWendel/node that referenced this issue 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: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
ErickWendel added a commit to ErickWendel/node that referenced this issue Dec 17, 2021
It fixes the problem for the child process not receiving messages.

Fixes: nodejs#41134
@Trott Trott closed this as completed in de12141 Dec 30, 2021
targos pushed a commit that referenced this issue Jan 14, 2022
It fixes the problem of the child process not receiving messages.

Fixes: #41134

PR-URL: #41221
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@Peeja
Copy link

Peeja commented Jan 29, 2022

For future searchers: This has been resolved and released in v17.4.0. (馃帀)

danielleadams pushed a commit that referenced this issue Jan 31, 2022
It fixes the problem of the child process not receiving messages.

Fixes: #41134

PR-URL: #41221
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
It fixes the problem of the child process not receiving messages.

Fixes: nodejs#41134

PR-URL: nodejs#41221
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
It fixes the problem of the child process not receiving messages.

Fixes: #41134

PR-URL: #41221
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@Methuselah96
Copy link

Also released in 16.14.0 if you're on LTS.

sublimator added a commit to interledger/web-monetization-projects that referenced this issue Apr 22, 2022
ErickWendel added a commit to ErickWendel/node that referenced this issue Apr 23, 2022
It fixes the problem of the child process not receiving messages.

Fixes: nodejs#41134
sublimator added a commit to interledger/web-monetization-projects that referenced this issue Apr 25, 2022
* chore: update yarn=4.0.0-rc.2 typescript=4.6.3

* fix(webmonetization-wext): alias Document['visibilityState']

* feat(ci): run yarn dlx @yarnpkg/sdks vscode

* chore: log the node version

* fix: set jest maxWorkers=1 to workaround nodejs/node#41134

* fix: properly pass args after --

* fix: run yarn dedupe

* fix: better method of configuring 1 jest worker

* fix: jest validates config, must be number or string

* feat: use node 18 and 16 LTS

* feat: use cimg/*

* feat: use specific version

* feat: remove maxWorkers workaround
juanarbol pushed a commit that referenced this issue May 1, 2022
It fixes the problem of the child process not receiving messages.

Fixes: #41134

PR-URL: #41221
Reviewed-By: Adrian Estrada <edsadr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Backport-PR-URL: #42840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants