Skip to content

Commit e65bed1

Browse files
addaleaxBridgeAR
addaleax
authored and
BridgeAR
committed
child_process: create proper public API for channel
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 <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent d6a2bad commit e65bed1

7 files changed

Lines changed: 100 additions & 23 deletions

File tree

‎doc/api/child_process.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1018,13 +1018,33 @@ See [Advanced Serialization][] for more details.
10181018
### `subprocess.channel`
10191019
<!-- YAML
10201020
added: v7.1.0
1021+
changes:
1022+
- version: REPLACEME
1023+
pr-url: https://github.com/nodejs/node/pull/30165
1024+
description: The object no longer accidentally exposes native C++ bindings.
10211025
-->
10221026

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

10251029
The `subprocess.channel` property is a reference to the child's IPC channel. If
10261030
no IPC channel currently exists, this property is `undefined`.
10271031

1032+
#### `subprocess.channel.ref()`
1033+
<!-- YAML
1034+
added: v7.1.0
1035+
-->
1036+
1037+
This method makes the IPC channel keep the event loop of the parent process
1038+
running if `.unref()` has been called before.
1039+
1040+
#### `subprocess.channel.unref()`
1041+
<!-- YAML
1042+
added: v7.1.0
1043+
-->
1044+
1045+
This method makes the IPC channel not keep the event loop of the parent process
1046+
running, and lets it finish even while the channel is open.
1047+
10281048
### `subprocess.connected`
10291049
<!-- YAML
10301050
added: v0.7.2

‎doc/api/process.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,10 @@ $ bash -c 'exec -a customArgv0 ./node'
626626
## `process.channel`
627627
<!-- YAML
628628
added: v7.1.0
629+
changes:
630+
- version: REPLACEME
631+
pr-url: https://github.com/nodejs/node/pull/30165
632+
description: The object no longer accidentally exposes native C++ bindings.
629633
-->
630634

631635
* {Object}
@@ -635,6 +639,30 @@ If the Node.js process was spawned with an IPC channel (see the
635639
property is a reference to the IPC channel. If no IPC channel exists, this
636640
property is `undefined`.
637641

642+
### `process.channel.ref()`
643+
<!-- YAML
644+
added: v7.1.0
645+
-->
646+
647+
This method makes the IPC channel keep the event loop of the process
648+
running if `.unref()` has been called before.
649+
650+
Typically, this is managed through the number of `'disconnect'` and `'message'`
651+
listeners on the `process` object. However, this method can be used to
652+
explicitly request a specific behavior.
653+
654+
### `process.channel.unref()`
655+
<!-- YAML
656+
added: v7.1.0
657+
-->
658+
659+
This method makes the IPC channel not keep the event loop of the process
660+
running, and lets it finish even while the channel is open.
661+
662+
Typically, this is managed through the number of `'disconnect'` and `'message'`
663+
listeners on the `process` object. However, this method can be used to
664+
explicitly request a specific behavior.
665+
638666
## `process.chdir(directory)`
639667
<!-- YAML
640668
added: v0.1.17

‎lib/child_process.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,10 +122,10 @@ function _forkChild(fd, serializationMode) {
122122
p.unref();
123123
const control = setupChannel(process, p, serializationMode);
124124
process.on('newListener', function onNewListener(name) {
125-
if (name === 'message' || name === 'disconnect') control.ref();
125+
if (name === 'message' || name === 'disconnect') control.refCounted();
126126
});
127127
process.on('removeListener', function onRemoveListener(name) {
128-
if (name === 'message' || name === 'disconnect') control.unref();
128+
if (name === 'message' || name === 'disconnect') control.unrefCounted();
129129
});
130130
}
131131

‎lib/internal/bootstrap/switches/is_main_thread.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,9 @@ function createWritableStdioStream(fd) {
9191
// an error when trying to use it again. In that case, create the socket
9292
// using the existing handle instead of the fd.
9393
if (process.channel && process.channel.fd === fd) {
94+
const { kChannelHandle } = require('internal/child_process');
9495
stream = new net.Socket({
95-
handle: process.channel,
96+
handle: process[kChannelHandle],
9697
readable: false,
9798
writable: true
9899
});

‎lib/internal/child_process.js‎

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ let freeParser;
6666
let HTTPParser;
6767

6868
const MAX_HANDLE_RETRANSMISSIONS = 3;
69+
const kChannelHandle = Symbol('kChannelHandle');
6970
const kIsUsedAsStdio = Symbol('kIsUsedAsStdio');
7071

7172
// This object contain function to convert TCP objects to native handle objects
@@ -108,7 +109,7 @@ const handleConversion = {
108109
// The worker should keep track of the socket
109110
message.key = socket.server._connectionKey;
110111

111-
const firstTime = !this.channel.sockets.send[message.key];
112+
const firstTime = !this[kChannelHandle].sockets.send[message.key];
112113
const socketList = getSocketList('send', this, message.key);
113114

114115
// The server should no longer expose a .connection property
@@ -508,30 +509,55 @@ ChildProcess.prototype.unref = function() {
508509
};
509510

510511
class Control extends EventEmitter {
512+
#channel = null;
513+
#refs = 0;
514+
#refExplicitlySet = false;
515+
511516
constructor(channel) {
512517
super();
513-
this.channel = channel;
514-
this.refs = 0;
518+
this.#channel = channel;
515519
}
516-
ref() {
517-
if (++this.refs === 1) {
518-
this.channel.ref();
520+
521+
// The methods keeping track of the counter are being used to track the
522+
// listener count on the child process object as well as when writes are
523+
// in progress. Once the user has explicitly requested a certain state, these
524+
// methods become no-ops in order to not interfere with the user's intentions.
525+
refCounted() {
526+
if (++this.#refs === 1 && !this.#refExplicitlySet) {
527+
this.#channel.ref();
519528
}
520529
}
521-
unref() {
522-
if (--this.refs === 0) {
523-
this.channel.unref();
530+
531+
unrefCounted() {
532+
if (--this.#refs === 0 && !this.#refExplicitlySet) {
533+
this.#channel.unref();
524534
this.emit('unref');
525535
}
526536
}
537+
538+
ref() {
539+
this.#refExplicitlySet = true;
540+
this.#channel.ref();
541+
}
542+
543+
unref() {
544+
this.#refExplicitlySet = true;
545+
this.#channel.unref();
546+
}
547+
548+
get fd() {
549+
return this.#channel ? this.#channel.fd : undefined;
550+
}
527551
}
528552

529553
const channelDeprecationMsg = '_channel is deprecated. ' +
530554
'Use ChildProcess.channel instead.';
531555

532556
let serialization;
533557
function setupChannel(target, channel, serializationMode) {
534-
target.channel = channel;
558+
const control = new Control(channel);
559+
target.channel = control;
560+
target[kChannelHandle] = channel;
535561

536562
ObjectDefineProperty(target, '_channel', {
537563
get: deprecate(() => {
@@ -547,8 +573,6 @@ function setupChannel(target, channel, serializationMode) {
547573
target._handleQueue = null;
548574
target._pendingMessage = null;
549575

550-
const control = new Control(channel);
551-
552576
if (serialization === undefined)
553577
serialization = require('internal/child_process/serialization');
554578
const {
@@ -796,11 +820,11 @@ function setupChannel(target, channel, serializationMode) {
796820

797821
if (wasAsyncWrite) {
798822
req.oncomplete = () => {
799-
control.unref();
823+
control.unrefCounted();
800824
if (typeof callback === 'function')
801825
callback(null);
802826
};
803-
control.ref();
827+
control.refCounted();
804828
} else if (typeof callback === 'function') {
805829
process.nextTick(callback, null);
806830
}
@@ -855,6 +879,7 @@ function setupChannel(target, channel, serializationMode) {
855879

856880
// This marks the fact that the channel is actually disconnected.
857881
this.channel = null;
882+
this[kChannelHandle] = null;
858883

859884
if (this._pendingMessage)
860885
closePendingHandle(this);
@@ -1011,7 +1036,7 @@ function getValidStdio(stdio, sync) {
10111036

10121037

10131038
function getSocketList(type, worker, key) {
1014-
const sockets = worker.channel.sockets[type];
1039+
const sockets = worker[kChannelHandle].sockets[type];
10151040
let socketList = sockets[key];
10161041
if (!socketList) {
10171042
const Construct = type === 'send' ? SocketListSend : SocketListReceive;
@@ -1054,6 +1079,7 @@ function spawnSync(options) {
10541079

10551080
module.exports = {
10561081
ChildProcess,
1082+
kChannelHandle,
10571083
setupChannel,
10581084
getValidStdio,
10591085
stdioStringToArray,

‎test/parallel/test-child-process-recv-handle.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ else
3636
function master() {
3737
// spawn() can only create one IPC channel so we use stdin/stdout as an
3838
// ad-hoc command channel.
39-
const proc = spawn(process.execPath, [__filename, 'worker'], {
39+
const proc = spawn(process.execPath, [
40+
'--expose-internals', __filename, 'worker'
41+
], {
4042
stdio: ['pipe', 'pipe', 'pipe', 'ipc']
4143
});
4244
let handle = null;
@@ -57,12 +59,13 @@ function master() {
5759
}
5860

5961
function worker() {
60-
process.channel.readStop(); // Make messages batch up.
62+
const { kChannelHandle } = require('internal/child_process');
63+
process[kChannelHandle].readStop(); // Make messages batch up.
6164
process.stdout.ref();
6265
process.stdout.write('ok\r\n');
6366
process.stdin.once('data', common.mustCall((data) => {
6467
assert.strictEqual(data.toString(), 'ok\r\n');
65-
process.channel.readStart();
68+
process[kChannelHandle].readStart();
6669
}));
6770
let n = 0;
6871
process.on('message', common.mustCall((msg, handle) => {

‎test/parallel/test-child-process-silent.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,7 @@ if (process.argv[2] === 'pipe') {
4242
const child = childProcess.fork(process.argv[1], ['pipe'], { silent: true });
4343

4444
// Allow child process to self terminate
45-
child.channel.close();
46-
child.channel = null;
45+
child.disconnect();
4746

4847
child.on('exit', function() {
4948
process.exit(0);

0 commit comments

Comments
 (0)