Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow options to be provided to child_process.send() #5283

Merged
merged 2 commits into from
Feb 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion doc/api/child_process.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -750,10 +750,11 @@ console.log(`Spawned child pid: ${grep.pid}`);
grep.stdin.end();
```

### child.send(message[, sendHandle][, callback])
### child.send(message[, sendHandle[, options]][, callback])

* `message` {Object}
* `sendHandle` {Handle}
* `options` {Object}
* `callback` {Function}
* Return: {Boolean}

Expand Down Expand Up @@ -801,6 +802,14 @@ passing a TCP server or socket object to the child process. The child will
receive the object as the second argument passed to the callback function
registered on the `process.on('message')` event.

The `options` argument, if present, is an object used to parameterize the
sending of certain types of handles. `options` supports the following
properties:

* `keepOpen` - A Boolean value that can be used when passing instances of
`net.Socket`. When `true`, the socket is kept open in the sending process.
Defaults to `false`.

The optional `callback` is a function that is invoked after the message is
sent but before the child may have received it. The function is called with a
single argument: `null` on success, or an [`Error`][] object on failure.
Expand Down
3 changes: 2 additions & 1 deletion doc/api/process.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -825,10 +825,11 @@ In custom builds from non-release versions of the source tree, only the
`name` property may be present. The additional properties should not be
relied upon to exist.

## process.send(message[, sendHandle][, callback])
## process.send(message[, sendHandle[, options]][, callback])

* `message` {Object}
* `sendHandle` {Handle object}
* `options` {Object}
* `callback` {Function}
* Return: {Boolean}

Expand Down
63 changes: 43 additions & 20 deletions lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const handleConversion = {
'net.Native': {
simultaneousAccepts: true,

send: function(message, handle) {
send: function(message, handle, options) {
return handle;
},

Expand All @@ -47,7 +47,7 @@ const handleConversion = {
'net.Server': {
simultaneousAccepts: true,

send: function(message, server) {
send: function(message, server, options) {
return server._handle;
},

Expand All @@ -60,7 +60,7 @@ const handleConversion = {
},

'net.Socket': {
send: function(message, socket) {
send: function(message, socket, options) {
if (!socket._handle)
return;

Expand All @@ -78,21 +78,25 @@ const handleConversion = {
if (firstTime) socket.server._setupSlave(socketList);

// Act like socket is detached
socket.server._connections--;
if (!options.keepOpen)
socket.server._connections--;
}

var handle = socket._handle;

// remove handle from socket object, it will be closed when the socket
// will be sent
var handle = socket._handle;
handle.onread = function() {};
socket._handle = null;
if (!options.keepOpen) {
handle.onread = function() {};
socket._handle = null;
}

return handle;
},

postSend: function(handle) {
postSend: function(handle, options) {
// Close the Socket handle after sending it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor nit: wouldn't it be slightly more efficient to:

if (options.keepOpen) return;
if (handle) handle.close();

/cc @bnoordhuis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like the idea of returning from the function early if options.keepOpen is true. That could require more refactoring if we ever decided to add more functionality here. It also makes the code less readable IMO. Also, 100% of legacy code and probably the majority of new code would have keepOpen = false, meaning that it would have to run two separate if statements instead of only checking keepOpen if handle is truthy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, good enough for me.

if (handle)
if (handle && !options.keepOpen)
handle.close();
},

Expand All @@ -117,7 +121,7 @@ const handleConversion = {
'dgram.Native': {
simultaneousAccepts: false,

send: function(message, handle) {
send: function(message, handle, options) {
return handle;
},

Expand All @@ -129,7 +133,7 @@ const handleConversion = {
'dgram.Socket': {
simultaneousAccepts: false,

send: function(message, socket) {
send: function(message, socket, options) {
message.dgramType = socket.type;

return socket._handle;
Expand Down Expand Up @@ -466,7 +470,7 @@ function setupChannel(target, channel) {
target._handleQueue = null;

queue.forEach(function(args) {
target._send(args.message, args.handle, false, args.callback);
target._send(args.message, args.handle, args.options, args.callback);
});

// Process a pending disconnect (if any).
Expand Down Expand Up @@ -498,13 +502,23 @@ function setupChannel(target, channel) {
});
});

target.send = function(message, handle, callback) {
target.send = function(message, handle, options, callback) {
if (typeof handle === 'function') {
callback = handle;
handle = undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that this allows a send(message, callback, options) signature.

options = undefined;
} else if (typeof options === 'function') {
callback = options;
options = undefined;
} else if (options !== undefined &&
(options === null || typeof options !== 'object')) {
throw new TypeError('"options" argument must be an object');
}

options = Object.assign({swallowErrors: false}, options);

if (this.connected) {
return this._send(message, handle, false, callback);
return this._send(message, handle, options, callback);
}
const ex = new Error('channel closed');
if (typeof callback === 'function') {
Expand All @@ -515,12 +529,17 @@ function setupChannel(target, channel) {
return false;
};

target._send = function(message, handle, swallowErrors, callback) {
target._send = function(message, handle, options, callback) {
assert(this.connected || this._channel);

if (message === undefined)
throw new TypeError('"message" argument cannot be undefined');

// Support legacy function signature
if (typeof options === 'boolean') {
options = {swallowErrors: options};
}

// package messages with a handle object
if (handle) {
// this message will be handled by an internalMessage event handler
Expand Down Expand Up @@ -549,6 +568,7 @@ function setupChannel(target, channel) {
this._handleQueue.push({
callback: callback,
handle: handle,
options: options,
message: message.msg,
});
return this._handleQueue.length === 1;
Expand All @@ -557,8 +577,10 @@ function setupChannel(target, channel) {
var obj = handleConversion[message.type];

// convert TCP object to native handle object
handle =
handleConversion[message.type].send.call(target, message, handle);
handle = handleConversion[message.type].send.call(target,
message,
handle,
options);

// If handle was sent twice, or it is impossible to get native handle
// out of it - just send a text without the handle.
Expand All @@ -575,6 +597,7 @@ function setupChannel(target, channel) {
this._handleQueue.push({
callback: callback,
handle: null,
options: options,
message: message,
});
return this._handleQueue.length === 1;
Expand All @@ -593,7 +616,7 @@ function setupChannel(target, channel) {
if (this.async === true)
control.unref();
if (obj && obj.postSend)
obj.postSend(handle);
obj.postSend(handle, options);
if (typeof callback === 'function')
callback(null);
};
Expand All @@ -605,9 +628,9 @@ function setupChannel(target, channel) {
} else {
// Cleanup handle on error
if (obj && obj.postSend)
obj.postSend(handle);
obj.postSend(handle, options);

if (!swallowErrors) {
if (!options.swallowErrors) {
const ex = errnoException(err, 'write');
if (typeof callback === 'function') {
process.nextTick(callback, ex);
Expand Down
52 changes: 52 additions & 0 deletions test/parallel/test-child-process-send-keep-open.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
const net = require('net');

if (process.argv[2] !== 'child') {
// The parent process forks a child process, starts a TCP server, and connects
// to the server. The accepted connection is passed to the child process,
// where the socket is written. Then, the child signals the parent process to
// write to the same socket.
let result = '';

process.on('exit', () => {
assert.strictEqual(result, 'childparent');
});

const child = cp.fork(__filename, ['child']);

// Verify that the child exits successfully
child.on('exit', common.mustCall((exitCode, signalCode) => {
assert.strictEqual(exitCode, 0);
assert.strictEqual(signalCode, null);
}));

const server = net.createServer((socket) => {
child.on('message', common.mustCall((msg) => {
assert.strictEqual(msg, 'child_done');
socket.end('parent', () => {
server.close();
child.disconnect();
});
}));

child.send('socket', socket, {keepOpen: true}, common.mustCall((err) => {
assert.ifError(err);
}));
});

server.listen(common.PORT, () => {
const socket = net.connect(common.PORT, common.localhostIPv4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call socket.setEncoding('utf8') if you're going to do string concatenation.

socket.setEncoding('utf8');
socket.on('data', (data) => result += data);
});
} else {
// The child process receives the socket from the parent, writes data to
// the socket, then signals the parent process to write
process.on('message', common.mustCall((msg, socket) => {
assert.strictEqual(msg, 'socket');
socket.write('child', () => process.send('child_done'));
}));
}
25 changes: 25 additions & 0 deletions test/parallel/test-child-process-send-type-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';
require('../common');
const assert = require('assert');
const cp = require('child_process');

function noop() {}

function fail(proc, args) {
assert.throws(() => {
proc.send.apply(proc, args);
}, /"options" argument must be an object/);
}

let target = process;

if (process.argv[2] !== 'child')
target = cp.fork(__filename, ['child']);

fail(target, ['msg', null, null]);
fail(target, ['msg', null, '']);
fail(target, ['msg', null, 'foo']);
fail(target, ['msg', null, 0]);
fail(target, ['msg', null, NaN]);
fail(target, ['msg', null, 1]);
fail(target, ['msg', null, null, noop]);