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: Add child_processes.isRootProcess flag #16258

Closed
arei opened this issue Oct 17, 2017 · 17 comments
Closed

child_process: Add child_processes.isRootProcess flag #16258

arei opened this issue Oct 17, 2017 · 17 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. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@arei
Copy link

arei commented Oct 17, 2017

  • Version: Future
  • Platform: All
  • Subsystem: child_process

This is a new feature request to add a flag (boolean) to child_process, called something along the lines of child_process.isRootProcess that would return true if the current process is the "root" of the child_process.fork tree or false if the current process was a forked process and has an upstream ipc channel.

In essence this is a simple one-liner inside of child_process:

child_process.isRootProcess = !!process.channel;

or something similar.

@Ginden
Copy link

Ginden commented Oct 17, 2017

Why put it in child_process instead of using directly !!process.channel?

@mscdex mscdex 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. labels Oct 17, 2017
@mscdex
Copy link
Contributor

mscdex commented Oct 17, 2017

I don't see the usefulness in adding this when you can just do if (process.send) or similar as you pointed out.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 17, 2017

I'm -1 on this. You should be able to check for process.send or process.connected.

@sindresorhus
Copy link

Readability is a good reason to add this. It's much clearer what if (child_process.isRootProcess) { checks than if (process.send) {.

@arei
Copy link
Author

arei commented Oct 17, 2017

We have cluster.isMaster and cluster.isWorker and this feature request is along the same lines. This is about providing readability into child_process as stated by @sindresorhus. I had to discover that I could do !!process.channel through web search and my own experimentation. Having the flag makes things much more accessible.

@sindresorhus
Copy link

I also had no idea you could check process.send or process.connected, and I've been using Node.js for 5 years.

@joyeecheung
Copy link
Member

Maybe a tip in the documentation would suffice?

@sindresorhus
Copy link

That doesn't really solve the readability concern though.

@bnoordhuis
Copy link
Member

We have cluster.isMaster and cluster.isWorker and this feature request is along the same lines.

For background: when cluster was introduced, there was no other way to feature-detect the type of process. It wasn't until v0.8 that you could do if (cluster.worker) { /* ... */ }

That rationale doesn't apply here. if (process.send) { /* ... */ } has worked from day one. (Well, at least since the day support was added for sending handles after spawning.)

Aside: .isRootProcess is a terrible name, it's bound to trip up UNIX users.

@refack
Copy link
Contributor

refack commented Oct 18, 2017

Welcome @arei, and thanks for the feedback. Based on the above comments I think it's a bit complicated to achieve. AFAICT a process.send or process.connected are both based on an IPC channel open between the processes, which is not guaranteed.
If you (or anyone wanting to help) could think of a more encompassing way to know this state, then opening a PR is probably the best way to get the most constructive reviews.

@arei
Copy link
Author

arei commented Oct 18, 2017

@bnoordhuis don’t disagree that the name could be better.

@EnoahNetzach
Copy link

I run into this problem once, and having to discover about the process.send/process. channel was kinda painful.
If then, as @refack says, it's not even guaranteed, that looks just as a workaround to me.

Would an API like child_process.isMaster, .isChild and possibly .isParent be better suited than the "root" one?

@apapirovski
Copy link
Member

I'm not seeing much movement but more importantly, the request basically seems to be for an alias of process.connected which is definitely documented: https://nodejs.org/dist/latest-v9.x/docs/api/process.html#process_process_connected

Unless someone has a proposal on how to proceed in the next few days, I'm going to close this out.

@apapirovski apapirovski self-assigned this Apr 13, 2018
@arei
Copy link
Author

arei commented Apr 13, 2018

I would be disappointed if this was closed. This absolutely boils down to a more succinct, documented approach to finding out if there is a parent node process or not. Yes, there is a way to do this already, but it is not clearly expressed. This feature request aims to solve that. And shouldn’t we be striving to make node more clearly express its intents?

@apapirovski
Copy link
Member

apapirovski commented Apr 13, 2018

There's no point to keeping issues open that are stale unless there's a clear path forward or there's great demand (with no dissent). Right now neither of those is true. Issues were brought up, e.g.

AFAICT a process.send or process.connected are both based on an IPC channel open between the processes, which is not guaranteed. If you (or anyone wanting to help) could think of a more encompassing way to know this state, then opening a PR is probably the best way to get the most constructive reviews.

and have not been addressed. As it is, we can't even put a label on this such as "help wanted" or "good first issue" because there's no path forward.

As @refack mentioned, if someone wants to open a PR or move this conversation forward then great but in the absence of that it just makes it really hard to manage the issue tracker.

@arei
Copy link
Author

arei commented Apr 13, 2018

Here's my proposed solution...

Any time a child_process is spawned, it adds an environment variable called NODE_TOP_LEVEL_PROCESS_PID. This will have the value of the top level node process.pid.

Add a function to process called isTopLevelNodeProcess() which will return true if NODE_TOP_LEVEL_PROCESS_PID is not found or null or matches the current process.pid value. Otherwise it will return false.

Thus all downstream spawned processes will return false, but the master process will return true.

I will submit a PR if I can ever get the project to build and test under windows.

@apapirovski apapirovski added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 14, 2018
@apapirovski apapirovski removed their assignment Apr 14, 2018
arei pushed a commit to arei/node that referenced this issue Apr 18, 2018
process.isTopLevelNodeProcess() will return true if the current process
was not originated by fork, spawn, or other like node execution. Otherwise
it will return false.

Fixes: nodejs#16258
@arei
Copy link
Author

arei commented Apr 19, 2018

Given the consensus is that this is not really a problem worth solving, I am closing it. I have also implemented this out in userland, although it is far from pretty. I mention it here for others whom may have the same problem or if anyone is interested in seeing: https://www.npmjs.com/package/toplevelprocess

@arei arei closed this as completed Apr 19, 2018
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. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests

10 participants