cluster: guard against undefined message handlers

[cjihrig]

Checklist

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

Affected core subsystem(s)

cluster

Description of change

cluster's internal message handling includes a cache of callback functions. Once the message for that callback is received, it is removed from the cache. If, for any reason, the same message ID is processed twice, the callback will be missing from the cache and cluster will try to call undefined as a function. This commit guards against this scenario.

Fixes#6561

[santigimeno]

What about adding a test based on the code from #6561 (comment)?

[cjihrig]

@santigimeno I could definitely do that. I wasn't sure if relying on undefined behavior (doing async things in the uncaughtException handler) in our tests was a great idea or not.

[santigimeno]

@cjihrig I understand. You're probably right.
As you explain, it looks like the worker is sending the same message twice. Shouldn't that be fixed as well?

[santigimeno]

Sorry, I meant, the worker is receiving the same message twice:

{"cmd":"NODE_CLUSTER","data":null,"ack":1,"key":":3000:4::0","errno":-48,"seq":0} 

[santigimeno]

@cjihrig I've tried a different fix: santigimeno@7dae5d7. Not too sure about it. What do you think?

[cjihrig]

@santigimeno I was just commenting. The problem, as your solution shows, is that the IPC channel doesn't get a chance to consume anything. I found the problem by adding a try...catchhere. I'm trying to see what the best way forward would be. I think the changes from this PR are still relevant though.

[santigimeno]

I think the changes from this PR are still relevant though.

Agreed

Something interesting: changing the scheduling policy to SCHED_NONE causes a different kind of error:

cluster.js:726 handle.owner.close(checkWaitingCount); ^ TypeError: Cannot read property 'close' of null at Worker._disconnect (cluster.js:726:19) at Worker.disconnect (cluster.js:686:17) at process.on.e (/Users/sgimeno/node/node/test_cluser.js:14:22) at emitOne (events.js:96:13) at process.emit (events.js:188:7) at process._fatalException (node.js:272:26) 

[cjihrig]

@santigimeno I opened #6909 as an alternative/complementary idea for this. I solved it in internal/child_process instead of cluster, as I think the problem is not specific to cluster, that's just where we observed it.

[cjihrig]

@santigimeno I came up with a test for this one. It fails for me before this change, and passes after. PTAL

[santigimeno]

@cjihrig I wonder if it's a good idea having this change as it may mask other hidden issues in the code (as #6909).

Anyway, the test itself LGTM

[cjihrig]

it may mask other hidden issues in the code

I definitely see where you're coming from with that comment, but I also think it would be good to make the code a little more robust. @bnoordhuis thoughts?

[bnoordhuis]

The change looks alright to me. LGTM.

[cjihrig]

CI: https://ci.nodejs.org/job/node-test-pull-request/2783/

[cjihrig]

CI was green minus what appears to be a Jenkins hiccup on ARM.

[MylesBorins]

@cjihrig lts?

[cjihrig]

Yes, please.