Child.send boolean #7739
Child.send boolean #7739
Conversation
nodejs-github-bot
added
child_process
|
I know this is a backport, but it seems there is an early EDIT: Looks like we need to pull in #3577 too. |
MylesBorins
force-pushed
the
v4.x-staging
branch
from
11da35d
to
63d94ee
Compare
The documentation indicates that child.send() returns a boolean but it has returned undefinined at since v0.12.0. It now returns a boolean per the (slightly updated) documentation. PR-URL: nodejs#3516 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
There are several places in the cluster module where a version of process.send() is called, but the result is swallowed. Most of these cases are internal, but Worker.prototype.send(), which is publicly documented, also suffers from this problem. This commit exposes the return value to facilitate better error handling, and bring Worker.prototype.send() into compliance with the documentation. PR-URL: nodejs#6998 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins
force-pushed
the
child.send-boolean
branch
from
ce048ca
to
b902fda
Compare
|
LGTM once #3577 is included, and the CI is happy. Obviously, it's up to the LTS team though. |
|
A soft -1 from me simply because this is a behaviour change that's nothing close to critical. The impact is fairly minimal, however, and I'm pretty sure it'd not be a breaking change to users in any way unless we uncover some weird edge case with it. |
|
hmmm, yeah, not sure about this. What's the benefit? |
|
main benefit in my opinion was being accurate to the docs that have been shipping since the start of v4 and remove a potentially odd / unexpected behavior difference between 4 / 6 At the same time I can respect the potential to break people, and that isn't in the spirit of LTS |
|
May be safer to fix the docs for LTS |
|
I'm also leaning -1 on this. |
|
I'm going to close this and opt for a doc fix. Thanks everyone! |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
lib, doc, cluster
Description of change
This is a backport of #3516 and #6998
Originally it was decided to not land #3516 into v4.x as it is semver minor. I would like to ping @nodejs/lts to reconsider as it would appear that the behavior change is accurate to the documentation. Further #6998 also makes a fix based on the documentation that relies on #3516
It may turn out that we do not wish to make a subtle change like this, especially if it can break people in prod. As such I will be running citgm against this to see if there are any obvious failures. If citgm + ci are both green I think we should consider backporting. It is inline with our documentation and will make subtle behavior equivalent in v4 and v6.