child_process: add public API for IPC channel #9322

cjihrig · 2016-10-27T18:40:17Z

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

sam-github · 2016-10-27T18:50:09Z

lib/internal/child_process.js

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


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.

sam-github · 2016-10-27T18:51:36Z

lib/internal/child_process.js

-  target._channel = channel;
+  target.channel = channel;
+
+  Object.defineProperty(target, '_channel', {


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

sam-github · 2016-10-27T18:52:07Z

Looks like a good direction to me.

jasnell · 2016-10-28T15:27:47Z

Seems worthwhile.

cjihrig · 2016-10-28T17:15:35Z

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

jasnell · 2016-10-28T17:17:22Z

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

sam-github · 2016-10-28T21:42:05Z

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.

jasnell

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

cjihrig · 2016-10-31T12:48:25Z

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 · 2016-10-31T18:53:53Z

@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 · 2016-10-31T19:15:53Z

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 · 2016-11-01T13:17:52Z

OK, added documentation for process.channel.

sam-github · 2016-11-01T15:09:55Z

doc/api/child_process.md

+* {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`.


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?

Updated to use property consistently.

sam-github · 2016-11-01T15:11:25Z

doc/api/process.md

+<!-- YAML
+added: REPLACEME
+-->
+


{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?

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 · 2016-11-01T15:12:17Z

LGTM

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

cjihrig · 2016-11-01T16:01:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4757/

cjihrig · 2016-11-01T19:39:49Z

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>

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>

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

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>

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

cjihrig mentioned this pull request Oct 27, 2016

child_process: add public API to unref ipc channel #9313

Closed

sam-github reviewed Oct 27, 2016

View reviewed changes

cjihrig force-pushed the channel branch from e84da6f to 3acdb94 Compare October 28, 2016 17:14

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

jasnell approved these changes Oct 30, 2016

View reviewed changes

cjihrig force-pushed the channel branch from 3acdb94 to 99b4958 Compare November 1, 2016 13:17

sam-github reviewed Nov 1, 2016

View reviewed changes

cjihrig force-pushed the channel branch from 99b4958 to a9b7820 Compare November 1, 2016 16:00

cjihrig force-pushed the channel branch from a9b7820 to 2dcb7f3 Compare November 2, 2016 00:38

cjihrig merged commit 2dcb7f3 into nodejs:master Nov 2, 2016

cjihrig deleted the channel branch November 2, 2016 00:44

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added the lts-agenda label Jan 6, 2017

MylesBorins added lts-watch-v4.x labels Jan 6, 2017

MylesBorins mentioned this pull request Jan 6, 2017

Potential Semver Minor Backports nodejs/Release#177

Closed

9 tasks

MylesBorins added dont-land-on-v4.x and removed lts-agenda labels Jan 16, 2017

addaleax mentioned this pull request Oct 29, 2019

child_process: create proper public API for channel #30165

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: add public API for IPC channel #9322

child_process: add public API for IPC channel #9322

cjihrig commented Oct 27, 2016 •

edited

Loading

sam-github Oct 27, 2016

sam-github Oct 27, 2016

sam-github commented Oct 27, 2016

jasnell commented Oct 28, 2016

cjihrig commented Oct 28, 2016

jasnell commented Oct 28, 2016

sam-github commented Oct 28, 2016

jasnell left a comment

cjihrig commented Oct 31, 2016

sam-github commented Oct 31, 2016

cjihrig commented Oct 31, 2016

cjihrig commented Nov 1, 2016

sam-github Nov 1, 2016

cjihrig Nov 1, 2016

sam-github Nov 1, 2016

cjihrig Nov 1, 2016

sam-github commented Nov 1, 2016

cjihrig commented Nov 1, 2016

cjihrig commented Nov 1, 2016

child_process: add public API for IPC channel #9322

child_process: add public API for IPC channel #9322

Conversation

cjihrig commented Oct 27, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

sam-github Oct 27, 2016

Choose a reason for hiding this comment

sam-github Oct 27, 2016

Choose a reason for hiding this comment

sam-github commented Oct 27, 2016

jasnell commented Oct 28, 2016

cjihrig commented Oct 28, 2016

jasnell commented Oct 28, 2016

sam-github commented Oct 28, 2016

jasnell left a comment

Choose a reason for hiding this comment

cjihrig commented Oct 31, 2016

sam-github commented Oct 31, 2016

cjihrig commented Oct 31, 2016

cjihrig commented Nov 1, 2016

sam-github Nov 1, 2016

Choose a reason for hiding this comment

cjihrig Nov 1, 2016

Choose a reason for hiding this comment

sam-github Nov 1, 2016

Choose a reason for hiding this comment

cjihrig Nov 1, 2016

Choose a reason for hiding this comment

sam-github commented Nov 1, 2016

cjihrig commented Nov 1, 2016

cjihrig commented Nov 1, 2016

cjihrig commented Oct 27, 2016 •

edited

Loading