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

child_process: special handling of process.on('message') #13913

Closed
refack opened this issue Jun 25, 2017 · 8 comments
Closed

child_process: special handling of process.on('message') #13913

refack opened this issue Jun 25, 2017 · 8 comments
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. process Issues and PRs related to the process subsystem.

Comments

@refack
Copy link
Contributor

refack commented Jun 25, 2017

  • Version: *
  • Platform: *
  • Subsystem: child_process
process.on('message');

has a special meaning in the context of IPC between parent and child. The problem is a 'message' event could be triggered other code, or listened to outside of IPC context, so we can not do any special treatment for it.
I suggest adding 'IPCMessage' that only the IPC channel can trigger, and registering a listener to would fail if an IPC channel was not established.
Ref: nodejs/help#693 (comment)
[edit]
The intention is to emit both events: message for backwards compatibility, and IPCMessage for a validated IPC only events.

@refack refack added child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. process Issues and PRs related to the process subsystem. labels Jun 25, 2017
@mscdex
Copy link
Contributor

mscdex commented Jun 25, 2017

-1 That would unnecessarily break backwards compatibility for zero gain IMHO.

@refack
Copy link
Contributor Author

refack commented Jun 25, 2017

-1 That would unnecessarily break backwards compatibility for zero gain IMHO.

@mscdex I mean emit both events (with proper checks). Should not break break compatibility, and allow for an opt-in stricter/guaranteed means of IPC.

@ORESoftware
Copy link
Contributor

@refack but couldn't some evil actor just emit IPCMessage

process.emit('IPCMessage') even if it were not an IPC message?

in my code, I check for properties on the message object coming back to see if it's IPC related.

@refack
Copy link
Contributor Author

refack commented Jun 26, 2017

@refack but couldn't some evil actor just emit IPCMessage
process.emit('IPCMessage') even if it were not an IPC message?
in my code, I check for properties on the message object coming back to see if it's IPC related.

As I see it the node internal code will have exclusivity on emitting IPCMessage. Probably need to inherit from EventEmitter and override the emit and the registerListener do validate IPCMessage events.
A really "evil actor" could probably monkey patch something, but that is true for most of the API, sometimes for the better (i.e. graceful-fs)

@mscdex
Copy link
Contributor

mscdex commented Jun 26, 2017

I still don't see any worthwhile gain coming from such changes.

@TimothyGu TimothyGu removed the good first issue Issues that are suitable for first-time contributors. label Jun 26, 2017
@TimothyGu
Copy link
Member

(removing "good first contribution" tag as there isn't yet a consensus)

@bnoordhuis
Copy link
Member

Also -1. Adds complexity and overhead for unclear benefit and feels a bit like a solution in search of a problem.

@refack
Copy link
Contributor Author

refack commented Jun 26, 2017

Closing for lack of consensus 🤷‍♂️

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. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants