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.

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

child_process: add public API to unref ipc channel #9313

Closed
mscdex opened this issue Oct 27, 2016 · 2 comments
Closed

child_process: add public API to unref ipc channel #9313

mscdex opened this issue Oct 27, 2016 · 2 comments
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. feature request Issues that request new features to be added to Node.js.

Comments

@mscdex
Copy link
Contributor

mscdex commented Oct 27, 2016 •
edited
Loading

  • Version: all
  • Platform: n/a
  • Subsystem: child_process

It would be nice to have a public API to unref() a child process's ipc channel. My use case for this is that I spawn a child process, send some messages back and forth via ipc, then at some point I want to detach the child process. child.unref() is not enough because that only unrefs the C++ ProcessWrap handle. Currently I have to resort to also doing child._channel.unref(). I am not sure if this should be done automatically inside child.unref() or if there should be a separate function or similar.
@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 27, 2016
@sam-github
Copy link
Contributor

I've unrefed _channel, too, for similar reasons, and other reasons.

I'm not sure ._channel should be quietly unrefed when you unref the ChildProcess. It seems inconsistent to me, because child#unref() doesn't unref the other pipes, like stdout, stderr, or even pipes on descriptors higher than 2.

I'd prefer for .channel to just be documented as a property that comes into existence when 'ipc' is in the stdio: options, as .stdout and the others are. This would mean changing the name of _channel, aliasing its name so both names exist, or just documenting _channel. I'd prefer aliasing in v7, deprecating in v8, and removing in v9.

@sam-github sam-github added doc Issues and PRs related to the documentations. docs-requested and removed doc Issues and PRs related to the documentations. labels Oct 27, 2016
@cjihrig cjihrig mentioned this issue Oct 27, 2016
Merged
4 tasks
@cjihrig
Copy link
Contributor

cjihrig commented Oct 27, 2016

I like that idea. I've opened #9322 as a potential starting point.

cjihrig added a commit to cjihrig/node that referenced this issue Nov 2, 2016
@cjihrig
child_process: add public API for IPC channel
2dcb7f3 
This commit adds a public channel property to ChildProcess. The
existing _channel property is aliased to the new property, with
the intention of deprecating and removing it in the future.

Fixes: nodejs#9313
PR-URL: nodejs#9322
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this issue Nov 3, 2016
@cjihrig @evanlucas
child_process: add public API for IPC channel
7b0e937 
This commit adds a public channel property to ChildProcess. The
existing _channel property is aliased to the new property, with
the intention of deprecating and removing it in the future.

Fixes: #9313
PR-URL: #9322
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. labels Dec 1, 2016
@addaleax addaleax mentioned this issue Oct 29, 2019
Closed
4 tasks
addaleax added a commit to addaleax/node that referenced this issue Nov 5, 2019
@addaleax
child_process: create proper public API for channel
cc7ff9e 
Instead of exposing the C++ bindings object as `subprocess.channel`
or `process.channel`, provide the “control” object that was
previously used internally as the public-facing variant of it.

This should be better than returning the raw pipe object, and
matches the original intention (when the `channel` property was
first added) of providing a proper way to `.ref()` or `.unref()`
the channel.

Refs: nodejs#9322
Refs: nodejs#9313
@frangio frangio mentioned this issue Nov 6, 2019
Merged
9 tasks
addaleax added a commit to addaleax/node that referenced this issue Jan 3, 2020
@addaleax
child_process: create proper public API for channel
ef418e1 
Instead of exposing the C++ bindings object as `subprocess.channel`
or `process.channel`, provide the “control” object that was
previously used internally as the public-facing variant of it.

This should be better than returning the raw pipe object, and
matches the original intention (when the `channel` property was
first added) of providing a proper way to `.ref()` or `.unref()`
the channel.

Refs: nodejs#9322
Refs: nodejs#9313
BridgeAR pushed a commit that referenced this issue Jan 3, 2020
@addaleax @BridgeAR
child_process: create proper public API for channel
e65bed1 
Instead of exposing the C++ bindings object as `subprocess.channel`
or `process.channel`, provide the “control” object that was
previously used internally as the public-facing variant of it.

This should be better than returning the raw pipe object, and
matches the original intention (when the `channel` property was
first added) of providing a proper way to `.ref()` or `.unref()`
the channel.

PR-URL: #30165
Refs: #9322
Refs: #9313
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@snyk-bot snyk-bot mentioned this issue Jan 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sam-github @mscdex @cjihrig