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

cjihrig · 2016-02-17T15:44:29Z

This PR adds an options argument to child_process.send() and process.send(). The second commit adds a keepOpen option that allows net.Socket instances to be kept open in multiple processes.

One limitation of the current implementation is that the new options argument can only be specified if the optional sendHandle argument is also provided. It's an unfortunate side effect of allowing multiple optional arguments of the same data type consecutively in function arguments. It can be worked around by duplicating code in _send(), but I'd rather save that for a semver-major change.

Closes #4271

jasnell · 2016-02-17T16:53:10Z

lib/internal/child_process.js


      return handle;
    },

-    postSend: function(handle) {
+    postSend: function(handle, options) {
      // Close the Socket handle after sending it


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

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

/cc @bnoordhuis

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.

ok, good enough for me.

jasnell · 2016-02-17T16:58:35Z

LGTM

cjihrig · 2016-02-20T16:35:02Z

@bnoordhuis would you mind taking a look at this?

bnoordhuis · 2016-02-21T11:24:18Z

lib/internal/child_process.js

-        socket.server._connections--;
+        if (!options.keepOpen)
+          socket.server._connections--;
+


Style nit: superfluous blank line.

cjihrig · 2016-02-21T14:13:11Z

@bnoordhuis nits addressed.

bnoordhuis · 2016-02-21T20:11:14Z

lib/internal/child_process.js

@@ -498,13 +502,22 @@ 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;


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

bnoordhuis · 2016-02-21T20:15:25Z

LGTM with a comment.

cjihrig · 2016-02-22T15:45:58Z

Thanks for the reviews. Addressed your most recent comment (set options = undefined in that case).

CI is yellow. One unrelated ARM failure on a flakey test. https://ci.nodejs.org/job/node-test-pull-request/1720/

This commit adds an options object to process.send(). The same object is propagated to process._send(), the _handleQueue, and the send() and postSend() functions of the handle converter. Fixes: nodejs#4271 PR-URL: nodejs#5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This option allows an instance of net.Socket to be kept open in the sending process. Fixes: nodejs#4271 PR-URL: nodejs#5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit adds an options object to process.send(). The same object is propagated to process._send(), the _handleQueue, and the send() and postSend() functions of the handle converter. Fixes: #4271 PR-URL: #5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This option allows an instance of net.Socket to be kept open in the sending process. Fixes: #4271 PR-URL: #5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit adds an options object to process.send(). The same object is propagated to process._send(), the _handleQueue, and the send() and postSend() functions of the handle converter. Fixes: #4271 PR-URL: #5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This option allows an instance of net.Socket to be kept open in the sending process. Fixes: #4271 PR-URL: #5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * child_process: “send()” now accepts an options parameter (cjihrig) #5283 - Currently the only option is “keepOpen”, which keeps the underlying socket open after the message is sent. * constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts) #5463 * Fixed two regressions which originated in v5.7.0: - http: Errors inside of http client callbacks now propagate correctly (Trevor Norris) #5591 - path: Fixed normalization of absolute paths (Evan Lucas) #5589 * repl: “start()” no longer requires an options parameter (cjihrig) #5388 * util: Improved “format()” performance 50-300% (Evan Lucas) #5360 PR-URL: #5559

Notable changes: * child_process: “send()” now accepts an options parameter (cjihrig) nodejs#5283 - Currently the only option is “keepOpen”, which keeps the underlying socket open after the message is sent. * constants: “ENGINE_METHOD_RSA” is now correctly exposed (Sam Roberts) nodejs#5463 * Fixed two regressions which originated in v5.7.0: - http: Errors inside of http client callbacks now propagate correctly (Trevor Norris) nodejs#5591 - path: Fixed normalization of absolute paths (Evan Lucas) nodejs#5589 * repl: “start()” no longer requires an options parameter (cjihrig) nodejs#5388 * util: Improved “format()” performance 50-300% (Evan Lucas) nodejs#5360 PR-URL: nodejs#5559

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. child_process Issues and PRs related to the child_process subsystem. labels Feb 17, 2016

jasnell reviewed Feb 17, 2016
View reviewed changes

cjihrig force-pushed the send-opts branch from 6aa1f30 to c3c9dd0 Compare February 17, 2016 17:21

bnoordhuis reviewed Feb 21, 2016
View reviewed changes

cjihrig force-pushed the send-opts branch from c3c9dd0 to 75db382 Compare February 21, 2016 14:10

bnoordhuis reviewed Feb 21, 2016
View reviewed changes

cjihrig force-pushed the send-opts branch from 75db382 to 558bb9f Compare February 22, 2016 14:14

cjihrig added 2 commits February 22, 2016 11:53

child_process: add keepOpen option to send()

e854f60

This option allows an instance of net.Socket to be kept open in the sending process. Fixes: nodejs#4271 PR-URL: nodejs#5283 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

cjihrig force-pushed the send-opts branch from 558bb9f to e854f60 Compare February 22, 2016 17:02

cjihrig merged commit e854f60 into nodejs:master Feb 22, 2016

cjihrig deleted the send-opts branch February 22, 2016 17:03

This was referenced Mar 1, 2016

Release proposal: v5.7.1 #5464

Merged

v5.8.0 proposal #5559

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cjihrig commented Feb 17, 2016

jasnell Feb 17, 2016

cjihrig Feb 18, 2016

jasnell Feb 19, 2016

jasnell commented Feb 17, 2016

cjihrig commented Feb 20, 2016

bnoordhuis Feb 21, 2016

cjihrig commented Feb 21, 2016

bnoordhuis Feb 21, 2016

bnoordhuis commented Feb 21, 2016

cjihrig commented Feb 22, 2016

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

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

Conversation

cjihrig commented Feb 17, 2016

jasnell Feb 17, 2016

Choose a reason for hiding this comment

cjihrig Feb 18, 2016

Choose a reason for hiding this comment

jasnell Feb 19, 2016

Choose a reason for hiding this comment

jasnell commented Feb 17, 2016

cjihrig commented Feb 20, 2016

bnoordhuis Feb 21, 2016

Choose a reason for hiding this comment

cjihrig commented Feb 21, 2016

bnoordhuis Feb 21, 2016

Choose a reason for hiding this comment

bnoordhuis commented Feb 21, 2016

cjihrig commented Feb 22, 2016