child_process: add callback parameter to .send() by bnoordhuis · Pull Request #2620 · nodejs/node

bnoordhuis · 2015-08-30T23:00:13Z

Add an optional callback parameter to ChildProcess.prototype.send()
that is invoked when the message has been sent.

Juggle the control channel's reference count so that in-flight messages
keep the event loop (and therefore the process) alive until they have
been sent.

ChildProcess.prototype.send() and process.send() used to operate
synchronously but became asynchronous in commit libuv/libuv@393c1c5
("unix: set non-block mode in uv_{pipe,tcp,udp}_open"), which landed
in io.js in commit 07bd05b ("deps: update libuv to 1.2.1").

Fixes: #760

R=@rvagg - note that I'm on holiday and may not be able to respond quickly if at all.

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/211/

rvagg · 2015-08-30T23:26:23Z

Looks great to me, two concerns: 1) the FIXME and 2) the ref/unref timing and potential for it to go below zero, obviously it shouldn't and I'm not suggesting your code has a way to do this but in the interests of defensiveness I wonder if the decrement shouldn't have a guard for < 0 because the ref is strictly when it reaches 1.

I'm happy with this going into v4, the doc changes are appreciated too. I'm also impressed this actually works on Windows! Some kind of black magic.

Fishrock123 · 2015-08-30T23:27:23Z

@bnoordhuis how does this interact with windows, where afaik you don't get this return info?

trevnorris · 2015-08-31T01:52:33Z

While here mind fixing placement of the first [?

ronkorving · 2015-08-31T03:27:10Z

Known issues in each changelog says:

process.send() is not synchronous as the docs suggest, a regression introduced in 1.0.2, see #760.

Does this mean that the intention is to keep it asynchronous? (in which case it's no longer a known issue, but rather BC breakage)

rvagg · 2015-08-31T03:28:38Z

yes and yes

ronkorving · 2015-08-31T03:35:28Z

Thanks 👍

cjihrig · 2015-08-31T12:22:47Z

Any reason not to just nextTick() this right now?

None that I can think of but it wouldn't be the first time a seemingly innocuous looking change breaks something. If we can get consensus, I'll change it.

After looking through the code base, there are actually many places where errors are emitted on the same tick. Maybe the FIXME should just be removed instead.

bnoordhuis · 2015-08-31T17:40:11Z

the ref/unref timing and potential for it to go below zero

That would be a logic error. I can add an assert if you want.

Incorporated @trevnorris's suggestions, PTAL (and good one on the process.nextTick() overload, I forgot that existed.)

sam-github · 2015-08-31T19:04:35Z

This impacts the cluster API and process API, which surface .send().

I assume process will now have an error event which can be emitted if send fails, and node will crash if it isn't listened on?

For users of cluster, if they fail to listen on worker.process.on('error', ..., the master will crash? Or will you re-emit from the process object onto the worker object?

There is no change in functionality - what's described here is the the current behavior, it just wasn't documented.

I thought send() after disconnect resulted in an error being thrown, not an error emitted, currently.

It emits an error event, but a synchronous one. So yes, if you don't have an error listener, it's effectively throwing an exception. That behavior is (for better or worse) maintained in this patch.

trevnorris · 2015-08-31T23:04:10Z

Nothing else from me. LGTM.

Add an optional callback parameter to `ChildProcess.prototype.send()` that is invoked when the message has been sent. Juggle the control channel's reference count so that in-flight messages keep the event loop (and therefore the process) alive until they have been sent. `ChildProcess.prototype.send()` and `process.send()` used to operate synchronously but became asynchronous in commit libuv/libuv@393c1c5 ("unix: set non-block mode in uv_{pipe,tcp,udp}_open"), which landed in io.js in commit 07bd05b ("deps: update libuv to 1.2.1").

bnoordhuis · 2015-09-01T22:14:49Z

https://ci.nodejs.org/job/node-accept-pull-request/96/

Fishrock123 · 2015-09-01T23:27:32Z

https://ci.nodejs.org/job/node-test-commit-arm/434/nodes=armv7-wheezy/tapTestReport/

not ok 838 - eval_messages.js
# [eval]
# [eval]:1
# with(this){__filename}
# ^^^^
# 
# SyntaxError: Strict mode code may not include a with statement
# at Object.exports.runInThisContext (vm.js:53:16)
# at Object.<anonymous> ([eval]-wrapper:6:22)
# at Module._compile (module.js:430:26)
# at node.js:566:27
# at doNTCallback0 (node.js:407:9)
# at process._tickCallback (node.js:336:13)
# 42
# 42

How the hell is that failing here all of a sudden??

rvagg · 2015-09-02T00:10:45Z

that output is normal output for eval_messages.js and doesn't necessarily indicate why it's failing, something else is going on to make it fail and the output isn't being very helpful

trevnorris · 2015-09-02T04:17:58Z

Can we just try CI again to see if we get lucky and it passes? Would be nice to get this landed.

orangemocha · 2015-09-02T07:57:34Z

Could the failure be related to this PR, or is that test just flaky?

trevnorris · 2015-09-02T09:11:41Z

@orangemocha Believe it's just a flaky test.

orangemocha · 2015-09-02T09:36:17Z

Okay, I opened #2648 to mark it so.

jasnell · 2015-09-02T16:56:56Z

Overall the change LGTM. Where are things at with regards to the failing CI? Were the failures unrelated?

trevnorris · 2015-09-02T21:43:07Z

CI again: https://ci.nodejs.org/job/node-merge-commit/155/

Add an optional callback parameter to `ChildProcess.prototype.send()` that is invoked when the message has been sent. Juggle the control channel's reference count so that in-flight messages keep the event loop (and therefore the process) alive until they have been sent. `ChildProcess.prototype.send()` and `process.send()` used to operate synchronously but became asynchronous in commit libuv/libuv@393c1c5 ("unix: set non-block mode in uv_{pipe,tcp,udp}_open"), which landed in io.js in commit 07bd05b ("deps: update libuv to 1.2.1"). Fixes: #760 PR-URL: #2620 Reviewed-By: trevnorris - Trevor Norris <trev.norris@gmail.com> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Aug 30, 2015

Fishrock123 added this to the 4.0.0 milestone Aug 30, 2015

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 30, 2015

trevnorris reviewed Aug 31, 2015
View reviewed changes

Comment thread doc/api/child_process.markdown Outdated

trevnorris Aug 31, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

While here mind fixing placement of the first [?

cjihrig reviewed Aug 31, 2015
View reviewed changes

sam-github reviewed Aug 31, 2015
View reviewed changes

pmuellr mentioned this pull request Sep 1, 2015

io.js + silent prepare issue alunny/node-xcode#61

Closed

bnoordhuis force-pushed the fix-760 branch from 7f0c8e2 to 99ad81a Compare September 1, 2015 22:13

orangemocha mentioned this pull request Sep 2, 2015

test: mark eval_messages as flaky #2648

Closed

rvagg mentioned this pull request Sep 7, 2015

Release proposal: v4.0.0 #2715

Merged

Zarel mentioned this pull request Dec 6, 2015

Cluster crash in main process, callback undefined when handling internal messages #3072

Closed

Uh oh!

Conversation

bnoordhuis commented Aug 30, 2015

Uh oh!

rvagg commented Aug 30, 2015

Uh oh!

Fishrock123 commented Aug 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronkorving commented Aug 31, 2015

Uh oh!

rvagg commented Aug 31, 2015

Uh oh!

ronkorving commented Aug 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Aug 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Aug 31, 2015

Uh oh!

bnoordhuis commented Sep 1, 2015

Uh oh!

Fishrock123 commented Sep 1, 2015

Uh oh!

rvagg commented Sep 2, 2015

Uh oh!

trevnorris commented Sep 2, 2015

Uh oh!

orangemocha commented Sep 2, 2015

Uh oh!

trevnorris commented Sep 2, 2015

Uh oh!

orangemocha commented Sep 2, 2015

Uh oh!

jasnell commented Sep 2, 2015

Uh oh!

trevnorris commented Sep 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants