child_process: don't crash process on internal ops

1. Swallow errors when sending internal NODE_HANDLE_ACK messages, so
   they don't crash the process.
2. Queue process.disconnect() if there are any pending queued messages.

Fixes test-child-process-fork-net2.js on win.

diff --git a/lib/child_process.js b/lib/child_process.js
index 75a637e..bcbd08d 100644 (file)
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -369,16 +369,25 @@ function setupChannel(target, channel) {
       assert(util.isArray(target._handleQueue));
       var queue = target._handleQueue;
       target._handleQueue = null;
+
       queue.forEach(function(args) {
-        target.send(args.message, args.handle);
+        target._send(args.message, args.handle, false);
       });
+
+      // Process a pending disconnect (if any).
+      if (!target.connected && target._channel && !target._handleQueue)
+        target._disconnect();
+
       return;
     }
 
     if (message.cmd !== 'NODE_HANDLE') return;
 
-    // Acknowledge handle receival.
-    target.send({ cmd: 'NODE_HANDLE_ACK' });
+    // Acknowledge handle receival. Don't emit error events (for example if
+    // the other side has disconnected) because this call to send() is not
+    // initiated by the user and it shouldn't be fatal to be unable to ACK
+    // a message.
+    target._send({ cmd: 'NODE_HANDLE_ACK' }, null, true);
 
     var obj = handleConversion[message.type];
 
@@ -395,14 +404,17 @@ function setupChannel(target, channel) {
   });
 
   target.send = function(message, handle) {
-    if (util.isUndefined(message)) {
-      throw new TypeError('message cannot be undefined');
-    }
-
-    if (!this.connected) {
+    if (!this.connected)
       this.emit('error', new Error('channel closed'));
-      return;
-    }
+    else
+      this._send(message, handle, false);
+  };
+
+  target._send = function(message, handle, swallowErrors) {
+    assert(this.connected || this._channel);
+
+    if (util.isUndefined(message))
+      throw new TypeError('message cannot be undefined');
 
     // package messages with a handle object
     if (handle) {
@@ -454,7 +466,8 @@ function setupChannel(target, channel) {
     var err = channel.writeUtf8String(req, string, handle);
 
     if (err) {
-      this.emit('error', errnoException(err, 'write'));
+      if (!swallowErrors)
+        this.emit('error', errnoException(err, 'write'));
     } else if (handle && !this._handleQueue) {
       this._handleQueue = [];
     }
@@ -467,15 +480,37 @@ function setupChannel(target, channel) {
     return channel.writeQueueSize < (65536 * 2);
   };
 
+  // connected will be set to false immediately when a disconnect() is
+  // requested, even though the channel might still be alive internally to
+  // process queued messages. The three states are distinguished as follows:
+  // - disconnect() never requested: _channel is not null and connected
+  //   is true
+  // - disconnect() requested, messages in the queue: _channel is not null
+  //   and connected is false
+  // - disconnect() requested, channel actually disconnected: _channel is
+  //   null and connected is false
   target.connected = true;
+
   target.disconnect = function() {
     if (!this.connected) {
       this.emit('error', new Error('IPC channel is already disconnected'));
       return;
     }
 
-    // do not allow messages to be written
+    // Do not allow any new messages to be written.
     this.connected = false;
+
+    // If there are no queued messages, disconnect immediately. Otherwise,
+    // postpone the disconnect so that it happens internally after the
+    // queue is flushed.
+    if (!this._handleQueue)
+      this._disconnect();
+  }
+
+  target._disconnect = function() {
+    assert(this._channel);
+
+    // This marks the fact that the channel is actually disconnected.
     this._channel = null;
 
     var fired = false;

diff --git a/test/simple/test-child-process-fork-net2.js b/test/simple/test-child-process-fork-net2.js
index a8e8039..098870d 100644 (file)
--- a/test/simple/test-child-process-fork-net2.js
+++ b/test/simple/test-child-process-fork-net2.js
@@ -52,8 +52,8 @@ if (process.argv[2] === 'child') {
       needEnd.push(socket);
     }
 
-    socket.on('close', function() {
-      console.error('[%d] socket.close', id, m);
+    socket.on('close', function(had_error) {
+      console.error('[%d] socket.close', id, had_error, m);
     });
 
     socket.on('finish', function() {
@@ -65,7 +65,7 @@ if (process.argv[2] === 'child') {
     if (m !== 'close') return;
     console.error('[%d] got close message', id);
     needEnd.forEach(function(endMe, i) {
-      console.error('[%d] ending %d', id, i);
+      console.error('[%d] ending %d/%d', id, i, needEnd.length);
       endMe.end('end');
     });
   });
@@ -73,7 +73,7 @@ if (process.argv[2] === 'child') {
   process.on('disconnect', function() {
     console.error('[%d] process disconnect, ending', id);
     needEnd.forEach(function(endMe, i) {
-      console.error('[%d] ending %d', id, i);
+      console.error('[%d] ending %d/%d', id, i, needEnd.length);
       endMe.end('end');
     });
   });
@@ -120,6 +120,11 @@ if (process.argv[2] === 'child') {
     var j = count, client;
     while (j--) {
       client = net.connect(common.PORT, '127.0.0.1');
+      client.on('error', function() {
+        // This can happen if we kill the child too early.
+        // The client should still get a close event afterwards.
+        console.error('[m] CLIENT: error event');
+      });
       client.on('close', function() {
         console.error('[m] CLIENT: close event');
         disconnected += 1;
@@ -136,6 +141,7 @@ if (process.argv[2] === 'child') {
     console.error('[m] server close');
     closeEmitted = true;
 
+    console.error('[m] killing child processes');
     child1.kill();
     child2.kill();
     child3.kill();