child_process: emit IPC messages on next tick

[cjihrig]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

Currently, if an IPC event handler throws an error, it can cause the message to not be consumed, leading to messages piling up. This commit causes IPC events to be emitted on the next tick, allowing the channel's processing logic to move forward as normal.

Refs: #6561
Refs: #6902

R= @santigimeno @bnoordhuis

[santigimeno]

I like this solution better.

LGTM if CI agrees.

[bnoordhuis]

LGTM. This is also a fix for #3072, I think? Is it possible to write a reliable regression test?

[santigimeno]

I have a test: santigimeno@c07eae8, that passes with this change, and never exits without it. Feel free to use it / modify it if it works for you.

[cjihrig]

Test LGTM. I'll pull it into this PR.

[jasnell]

LGTM with the test added and green CI

[bnoordhuis]

Test LGTM2. @cjihrig Consider stress-testing it before you land the PR.

[cjihrig]

Normal CI: https://ci.nodejs.org/job/node-test-pull-request/2742/. Will stress test next.

[cjihrig]

The test timed out on Windows due to the different scheduling policy. I've added another commit, which addresses it, but for whatever reason, GitHub isn't currently showing it.

Here it is. @bnoordhuis and @santigimeno what do you think?

--- a/lib/cluster.js
+++ b/lib/cluster.js
@@ -719,7 +719,11 @@ function workerInit() {
       const handle = handles[key];
       delete handles[key];
       waitingCount++;
-      handle.owner.close(checkWaitingCount);
+
+      if (handle.owner)
+        handle.owner.close(checkWaitingCount);
+      else
+        handle.close(checkWaitingCount);
     }

     checkWaitingCount();

[cjihrig]

GitHub seems to be working again. CI after latest commit: https://ci.nodejs.org/job/node-test-pull-request/2744/

[cjihrig]

CI is green, except for one PPC machine that seems hung or something.

[santigimeno]

LGTM.

The last commit seems to fix a different issue though. A couple of comments:

• Is SCHED_RR usable on Windows? or is it simply not recommended? If the latter, maybe the SchedulingPolicy of the cluster could be set unconditionally to RR in the test so it works the same on every platform.
• Maybe it's worth adding a different test that exercises the ownerless handles problem on all the platforms, not only on Windows. I have created this if you want to use it: santigimeno@e8f284b

[cjihrig]

CI with all four commits: https://ci.nodejs.org/job/node-test-pull-request/2765/

[cjihrig]

CI was all green except for one flake (test-stdout-close-catch - #6918).

Stress test (Ubuntu): https://ci.nodejs.org/job/node-stress-single-test/737/
Stress test (Windows): https://ci.nodejs.org/job/node-stress-single-test/738/

[bnoordhuis]

LGTM. Looks like a flake on the fbsd10 buildbot.

[cjihrig]

Thanks all. Landed in d59917b, dd21bd9, aadfe6c, and f0a07d9.

[MylesBorins]

@cjihrig lts?

[cjihrig]

IMO yes. It keeps the IPC channel from getting constipated.