From cc7ff9e8a5c466ebf7bf4175735dde8015c972d1 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 29 Oct 2019 20:15:31 +0100
Subject: [PATCH] child_process: create proper public API for `channel`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/nodejs/node/pull/9322
Refs: https://github.com/nodejs/node/issues/9313
---
 doc/api/child_process.md                      | 20 +++++++
 doc/api/process.md                            | 28 +++++++++
 lib/child_process.js                          |  4 +-
 lib/internal/child_process.js                 | 58 ++++++++++++++-----
 lib/internal/process/stdio.js                 |  9 +--
 .../test-child-process-recv-handle.js         |  9 ++-
 test/parallel/test-child-process-silent.js    |  3 +-
 7 files changed, 104 insertions(+), 27 deletions(-)

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index a2945ad7647716..bbd998a0a91a2f 100644
--- a/doc/api/child_process.md
+++ b/doc/api/child_process.md
@@ -1018,6 +1018,10 @@ See [Advanced Serialization][] for more details.
 ### subprocess.channel
 <!-- YAML
 added: v7.1.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/????
+    description: The object no longer accidentally exposes native C++ bindings.
 -->
 
 * {Object} A pipe representing the IPC channel to the child process.
@@ -1025,6 +1029,22 @@ added: v7.1.0
 The `subprocess.channel` property is a reference to the child's IPC channel. If
 no IPC channel currently exists, this property is `undefined`.
 
+### subprocess.channel.ref()
+<!-- YAML
+added: v7.1.0
+-->
+
+This method makes the IPC channel keep the event loop of the parent process
+running if `.unref()` has been called before.
+
+### subprocess.channel.unref()
+<!-- YAML
+added: v7.1.0
+-->
+
+This method makes the IPC channel not keep the event loop of the parent process
+running, and lets it finish even while the channel is open.
+
 ### subprocess.connected
 <!-- YAML
 added: v0.7.2
diff --git a/doc/api/process.md b/doc/api/process.md
index a9b5c4af09215b..458ae594b7af4d 100644
--- a/doc/api/process.md
+++ b/doc/api/process.md
@@ -624,6 +624,10 @@ $ bash -c 'exec -a customArgv0 ./node'
 ## process.channel
 <!-- YAML
 added: v7.1.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/????
+    description: The object no longer accidentally exposes native C++ bindings.
 -->
 
 * {Object}
@@ -633,6 +637,30 @@ If the Node.js process was spawned with an IPC channel (see the
 property is a reference to the IPC channel. If no IPC channel exists, this
 property is `undefined`.
 
+### process.channel.ref()
+<!-- YAML
+added: v7.1.0
+-->
+
+This method makes the IPC channel keep the event loop of the process
+running if `.unref()` has been called before.
+
+Typically, this is managed through the number of `'disconnect'` and `'message'`
+listeners on the `process` object. However, this method can be used to
+explicitly request a specific behavior.
+
+### process.channel.unref()
+<!-- YAML
+added: v7.1.0
+-->
+
+This method makes the IPC channel not keep the event loop of the process
+running, and lets it finish even while the channel is open.
+
+Typically, this is managed through the number of `'disconnect'` and `'message'`
+listeners on the `process` object. However, this method can be used to
+explicitly request a specific behavior.
+
 ## process.chdir(directory)
 <!-- YAML
 added: v0.1.17
diff --git a/lib/child_process.js b/lib/child_process.js
index 16dc30856a96f5..f708e91f91d7ed 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -115,10 +115,10 @@ function _forkChild(fd, serializationMode) {
   p.unref();
   const control = setupChannel(process, p, serializationMode);
   process.on('newListener', function onNewListener(name) {
-    if (name === 'message' || name === 'disconnect') control.ref();
+    if (name === 'message' || name === 'disconnect') control.refCounted();
   });
   process.on('removeListener', function onRemoveListener(name) {
-    if (name === 'message' || name === 'disconnect') control.unref();
+    if (name === 'message' || name === 'disconnect') control.unrefCounted();
   });
 }
 
diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 9e13650fa3d56a..0b95c4c0569f21 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -61,6 +61,7 @@ let freeParser;
 let HTTPParser;
 
 const MAX_HANDLE_RETRANSMISSIONS = 3;
+const kChannelHandle = Symbol('kChannelHandle');
 const kIsUsedAsStdio = Symbol('kIsUsedAsStdio');
 
 // This object contain function to convert TCP objects to native handle objects
@@ -103,8 +104,8 @@ const handleConversion = {
         // The worker should keep track of the socket
         message.key = socket.server._connectionKey;
 
-        var firstTime = !this.channel.sockets.send[message.key];
-        var socketList = getSocketList('send', this, message.key);
+        const firstTime = !this[kChannelHandle].sockets.send[message.key];
+        const socketList = getSocketList('send', this, message.key);
 
         // The server should no longer expose a .connection property
         // and when asked to close it should query the socket status from
@@ -503,22 +504,45 @@ ChildProcess.prototype.unref = function() {
 };
 
 class Control extends EventEmitter {
+  #channel = null;
+  #refs = 0;
+  #refExplicitlySet = false;
+
   constructor(channel) {
     super();
-    this.channel = channel;
-    this.refs = 0;
+    this.#channel = channel;
   }
-  ref() {
-    if (++this.refs === 1) {
-      this.channel.ref();
+
+  // The methods keeping track of the counter are being used to track the
+  // listener count on the child process object as well as when writes are
+  // in progress. Once the user has explicitly requested a certain state, these
+  // methods become no-ops in order to not interfere with the user's intentions.
+  refCounted() {
+    if (++this.#refs === 1 && !this.#refExplicitlySet) {
+      this.#channel.ref();
     }
   }
-  unref() {
-    if (--this.refs === 0) {
-      this.channel.unref();
+
+  unrefCounted() {
+    if (--this.#refs === 0 && !this.#refExplicitlySet) {
+      this.#channel.unref();
       this.emit('unref');
     }
   }
+
+  ref() {
+    this.#refExplicitlySet = true;
+    this.#channel.ref();
+  }
+
+  unref() {
+    this.#refExplicitlySet = true;
+    this.#channel.unref();
+  }
+
+  get fd() {
+    return this.#channel ? this.#channel.fd : undefined;
+  }
 }
 
 const channelDeprecationMsg = '_channel is deprecated. ' +
@@ -526,7 +550,9 @@ const channelDeprecationMsg = '_channel is deprecated. ' +
 
 let serialization;
 function setupChannel(target, channel, serializationMode) {
-  target.channel = channel;
+  const control = new Control(channel);
+  target.channel = control;
+  target[kChannelHandle] = channel;
 
   Object.defineProperty(target, '_channel', {
     get: deprecate(() => {
@@ -542,8 +568,6 @@ function setupChannel(target, channel, serializationMode) {
   target._handleQueue = null;
   target._pendingMessage = null;
 
-  const control = new Control(channel);
-
   if (serialization === undefined)
     serialization = require('internal/child_process/serialization');
   const {
@@ -791,11 +815,11 @@ function setupChannel(target, channel, serializationMode) {
 
       if (wasAsyncWrite) {
         req.oncomplete = () => {
-          control.unref();
+          control.unrefCounted();
           if (typeof callback === 'function')
             callback(null);
         };
-        control.ref();
+        control.refCounted();
       } else if (typeof callback === 'function') {
         process.nextTick(callback, null);
       }
@@ -850,6 +874,7 @@ function setupChannel(target, channel, serializationMode) {
 
     // This marks the fact that the channel is actually disconnected.
     this.channel = null;
+    this[kChannelHandle] = null;
 
     if (this._pendingMessage)
       closePendingHandle(this);
@@ -1006,7 +1031,7 @@ function getValidStdio(stdio, sync) {
 
 
 function getSocketList(type, worker, key) {
-  const sockets = worker.channel.sockets[type];
+  const sockets = worker[kChannelHandle].sockets[type];
   var socketList = sockets[key];
   if (!socketList) {
     var Construct = type === 'send' ? SocketListSend : SocketListReceive;
@@ -1049,6 +1074,7 @@ function spawnSync(options) {
 
 module.exports = {
   ChildProcess,
+  kChannelHandle,
   setupChannel,
   getValidStdio,
   stdioStringToArray,
diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js
index 08781547c48952..09560199ec0595 100644
--- a/lib/internal/process/stdio.js
+++ b/lib/internal/process/stdio.js
@@ -67,16 +67,17 @@ function getMainThreadStdio() {
         break;
 
       case 'PIPE':
-      case 'TCP':
-        var net = require('net');
+      case 'TCP': {
+        const net = require('net');
 
         // It could be that process has been started with an IPC channel
         // sitting on fd=0, in such case the pipe for this fd is already
         // present and creating a new one will lead to the assertion failure
         // in libuv.
         if (process.channel && process.channel.fd === fd) {
+          const kChannelHandle = require('internal/child_process');
           stdin = new net.Socket({
-            handle: process.channel,
+            handle: process[kChannelHandle],
             readable: true,
             writable: false,
             manualStart: true
@@ -92,7 +93,7 @@ function getMainThreadStdio() {
         // Make sure the stdin can't be `.end()`-ed
         stdin._writableState.ended = true;
         break;
-
+      }
       default:
         // Provide a dummy contentless input for e.g. non-console
         // Windows applications.
diff --git a/test/parallel/test-child-process-recv-handle.js b/test/parallel/test-child-process-recv-handle.js
index 24c6b07037f2d0..bc31ee6bf025eb 100644
--- a/test/parallel/test-child-process-recv-handle.js
+++ b/test/parallel/test-child-process-recv-handle.js
@@ -36,7 +36,9 @@ else
 function master() {
   // spawn() can only create one IPC channel so we use stdin/stdout as an
   // ad-hoc command channel.
-  const proc = spawn(process.execPath, [__filename, 'worker'], {
+  const proc = spawn(process.execPath, [
+    '--expose-internals', __filename, 'worker'
+  ], {
     stdio: ['pipe', 'pipe', 'pipe', 'ipc']
   });
   let handle = null;
@@ -57,12 +59,13 @@ function master() {
 }
 
 function worker() {
-  process.channel.readStop();  // Make messages batch up.
+  const { kChannelHandle } = require('internal/child_process');
+  process[kChannelHandle].readStop();  // Make messages batch up.
   process.stdout.ref();
   process.stdout.write('ok\r\n');
   process.stdin.once('data', common.mustCall((data) => {
     assert.strictEqual(data.toString(), 'ok\r\n');
-    process.channel.readStart();
+    process[kChannelHandle].readStart();
   }));
   let n = 0;
   process.on('message', common.mustCall((msg, handle) => {
diff --git a/test/parallel/test-child-process-silent.js b/test/parallel/test-child-process-silent.js
index a2ebb3ffeb9385..bedef1509cf9c7 100644
--- a/test/parallel/test-child-process-silent.js
+++ b/test/parallel/test-child-process-silent.js
@@ -42,8 +42,7 @@ if (process.argv[2] === 'pipe') {
   const child = childProcess.fork(process.argv[1], ['pipe'], { silent: true });
 
   // Allow child process to self terminate
-  child.channel.close();
-  child.channel = null;
+  child.disconnect();
 
   child.on('exit', function() {
     process.exit(0);