From 0da4e0e8439a453d197484402f9f14798c6e00d0 Mon Sep 17 00:00:00 2001 From: Alexis Campailla Date: Thu, 21 Nov 2013 18:49:41 -0800 Subject: [PATCH] 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. --- lib/child_process.js | 59 +++++++++++++++++++++++------ test/simple/test-child-process-fork-net2.js | 14 +++++-- 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 75a637e..bcbd08d 100644 --- 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 --- 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(); -- 2.7.4