child_process: fix handle passing with large payloads #14588
Conversation
nodejs-github-bot
added
the
child_process
| process.on('message', common.mustCall(({ payload: received }, handle) => { | ||
| assert.strictEqual(payload, received); | ||
| assert(handle instanceof net.Socket); | ||
| process.send({ payload: received }, handle); |
There was a problem hiding this comment.
Nit: use { payload } for consistency?
lib/internal/child_process.js
Outdated
| } else { | ||
| handleMessage(message, undefined, false); | ||
| } | ||
|
|
||
| pendingHandle = null; |
There was a problem hiding this comment.
This looks incorrect: it clears the variable on the first iteration but what if the NODE_HANDLE message is not in the first chunk? I would have expected to see this assignment in the if (message.cmd === 'NODE_HANDLE') { block.
There was a problem hiding this comment.
Right, I think that makes more sense – done.
addaleax
force-pushed
the
cluster-large-message-handle
branch
2 times, most recently
from
8f3c0e0 to
2f65c3f
Compare
Fix situations in which the handle passed along with a message that has a large payload and can’t be read entirely by a single `recvmsg()` call isn’t associated with the message to which it belongs. Fixes: nodejs#13778
addaleax
force-pushed
the
cluster-large-message-handle
branch
from
2f65c3f to
be0fed0
Compare
|
So … the test here isn’t even flaky on Windows, it’s just always failing. Would anybody in @nodejs/platform-windows have a chance to look at what’s going wrong? |
|
Tried something new with help from @tniessen CI: https://ci.nodejs.org/job/node-test-commit/11599/ |
|
Landed in 611851d |
Fix situations in which the handle passed along with a message that has a large payload and can’t be read entirely by a single `recvmsg()` call isn’t associated with the message to which it belongs. PR-URL: #14588 Fixes: #13778 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
|
This is going to need a manual backport for both v4 and v6 |
Fix situations in which the handle passed along with a message
that has a large payload and can’t be read entirely by a single
recvmsg()call isn’t associated with the message to which itbelongs.
Fixes: #13778
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
child_process