Skip to content

Commit

Permalink
child_process: workaround fd passing issue on OS X
Browse files Browse the repository at this point in the history
There's an issue on some `OS X` versions when passing fd's between processes.
When the handle associated to a specific file descriptor is closed by the sender
process before it's received in the destination, the handle is indeed closed
while it should remain opened. In order to fix this behaviour, don't close the
handle until the `NODE_HANDLE_ACK` is received by the sender.
Added `test-child-process-pass-fd` that is basically `test-cluster-net-send` but
creating lots of workers, so the issue reproduces on `OS X` consistently.

Fixes: #7512
Ref: #8904
PR-URL: #7572
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
  • Loading branch information
santigimeno authored and MylesBorins committed Oct 26, 2016
1 parent 6effc4a commit 5ed5142
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 8 deletions.
41 changes: 33 additions & 8 deletions lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,21 @@ const handleConversion = {
return handle;
},

postSend: function(handle) {
// Close the Socket handle after sending it
if (handle)
handle.close();
postSend: function(handle, target) {
// Store the handle after successfully sending it, so it can be closed
// when the NODE_HANDLE_ACK is received. If the handle could not be sent,
// just close it.
if (handle) {
if (target) {
// There can only be one _pendingHandle as passing handles are
// processed one at a time: handles are stored in _handleQueue while
// waiting for the NODE_HANDLE_ACK of the current passing handle.
assert(!target._pendingHandle);
target._pendingHandle = handle;
} else {
handle.close();
}
}
},

got: function(message, handle, emit) {
Expand Down Expand Up @@ -396,6 +407,7 @@ ChildProcess.prototype.unref = function() {
function setupChannel(target, channel) {
target._channel = channel;
target._handleQueue = null;
target._pendingHandle = null;

const control = new class extends EventEmitter {
constructor() {
Expand Down Expand Up @@ -461,6 +473,11 @@ function setupChannel(target, channel) {
target.on('internalMessage', function(message, handle) {
// Once acknowledged - continue sending handles.
if (message.cmd === 'NODE_HANDLE_ACK') {
if (target._pendingHandle) {
target._pendingHandle.close();
target._pendingHandle = null;
}

assert(Array.isArray(target._handleQueue));
var queue = target._handleQueue;
target._handleQueue = null;
Expand Down Expand Up @@ -587,13 +604,16 @@ function setupChannel(target, channel) {
var err = channel.writeUtf8String(req, string, handle);

if (err === 0) {
if (handle && !this._handleQueue)
this._handleQueue = [];
if (handle) {
if (!this._handleQueue)
this._handleQueue = [];
if (obj && obj.postSend)
obj.postSend(handle, target);
}

req.oncomplete = function() {
if (this.async === true)
control.unref();
if (obj && obj.postSend)
obj.postSend(handle);
if (typeof callback === 'function')
callback(null);
};
Expand Down Expand Up @@ -654,6 +674,11 @@ function setupChannel(target, channel) {
// This marks the fact that the channel is actually disconnected.
this._channel = null;

if (this._pendingHandle) {
this._pendingHandle.close();
this._pendingHandle = null;
}

var fired = false;
function finish() {
if (fired) return;
Expand Down
53 changes: 53 additions & 0 deletions test/sequential/test-child-process-pass-fd.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fork = require('child_process').fork;
const net = require('net');

if ((process.config.variables.arm_version === '6') ||
(process.config.variables.arm_version === '7')) {
common.skip('Too slow for armv6 and armv7 bots');
return;
}

const N = 80;

if (process.argv[2] !== 'child') {
for (let i = 0; i < N; ++i) {
const worker = fork(__filename, ['child', common.PORT + i]);
worker.once('message', common.mustCall((msg, handle) => {
assert.strictEqual(msg, 'handle');
assert.ok(handle);
worker.send('got');

let recvData = '';
handle.on('data', common.mustCall((data) => {
recvData += data;
}));

handle.on('end', () => {
assert.strictEqual(recvData, 'hello');
worker.kill();
});
}));
}
} else {
let socket;
const port = process.argv[3];
let cbcalls = 0;
function socketConnected() {
if (++cbcalls === 2)
process.send('handle', socket);
}

const server = net.createServer((c) => {
process.once('message', function(msg) {
assert.strictEqual(msg, 'got');
c.end('hello');
});
socketConnected();
});
server.listen(port, common.localhostIPv4, () => {
socket = net.connect(port, common.localhostIPv4, socketConnected);
});
}

0 comments on commit 5ed5142

Please sign in to comment.