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 public API for IPC channel #9322

Merged
merged 1 commit into from
Nov 2, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 27, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

Description of change

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.

I will add the documentation if we decide to move forward with this.

Fixes: #9313

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 27, 2016

Object.defineProperty(target, '_channel', {
get() { return target.channel; },
set(val) { target.channel = val; },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting it is DEEP into undefined behaviour, I guess its backwards compatible... but I can't think of a reason to take a shot-gun to the ChildProcess internals.

target._channel = channel;
target.channel = channel;

Object.defineProperty(target, '_channel', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a note that this can be deprecated in node 8.x?

@sam-github
Copy link
Contributor

Looks like a good direction to me.

@jasnell
Copy link
Member

jasnell commented Oct 28, 2016

Seems worthwhile.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 28, 2016

OK, added docs and a code comment allowing deprecation in version 8.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 28, 2016
@jasnell
Copy link
Member

jasnell commented Oct 28, 2016

@ChALkeR ... can you do a search for uses of _channel?

@sam-github
Copy link
Contributor

I just looked for my own uses, and I believe this change also applies/creates process.channel in fork/cluster/spawn-with-ipc children, so it should be documented, too. At least, I have places where I have to do process._channel.unref(). Maybe cluster workers, as well, though they inherit most of their attributes from ChildProcess, so it might not show up for them.

I'd love to know how @ChALkeR does his magic,btw, but looking at my code, I think a number of my uses are in test code, which might not show up on npmjs.org.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the additional doc that @sam-github suggests.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 31, 2016

I'm not sure of the best way to add documentation for those other things. The channel property is only relevant to ChildProcess objects. fork() and spawn() already mention that they return a ChildProcess, and cluster.worker.process is already documented as a ChildProcess.

@sam-github
Copy link
Contributor

@cjihrig are you sure that there is no process._channel under the conditions that there are a process.send() https://nodejs.org/docs/latest/api/process.html#process_process_send_message_sendhandle_options_callback

That setup target function that you modified is, IIRC (but its been a year), also called with process as target in a node child spawned with an IPC channel.

There might be a cluster.fork()._channel as well, and after this change, a cluster.fork().channel.

And if there isn't the two APIs above... maybe there should be?

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 31, 2016

Ah, yea process._channel exists in the child process, so that can be added.

In cluster it's not there, but cluster.fork().process._channel is there, and the process property is documented.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 1, 2016

OK, added documentation for process.channel.

* {Object} A pipe representing the IPC channel to the child process.

The `child.channel` property is a reference to the child's IPC channel. If no
IPC channel currently exists, this value is `undefined`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the property is undefined

maybe? It seems odd to me to say "The X property is blah, this value is undefined", why speak of a property in the first sentence, and a value in the second?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use property consistently.

<!-- YAML
added: REPLACEME
-->

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • {Object} A pipe representing the IPC channel to the child process.

Not needed here, the style of docs is different in child_process and process?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed that the style of the docs in this file are slightly different. I think a good first contribution would be to bring additional consistency to this file. For example, see https://nodejs.org/api/process.html#process_process_connected vs. https://nodejs.org/api/child_process.html#child_process_child_connected.

@sam-github
Copy link
Contributor

LGTM

Some minor comments on the doc wording, feel free to address if you agree, or leave as-is if you don't.

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 1, 2016

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 1, 2016

CI failures all looked infrastructure related, but going to try to get a better run: https://ci.nodejs.org/job/node-test-pull-request/4758/

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>
@cjihrig cjihrig merged commit 2dcb7f3 into nodejs:master Nov 2, 2016
@cjihrig cjihrig deleted the channel branch November 2, 2016 00:44
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
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>
addaleax added a commit to addaleax/node that referenced this pull request Nov 5, 2019
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
addaleax added a commit to addaleax/node that referenced this pull request Jan 3, 2020
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 pull request Jan 3, 2020
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>
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants