cluster: guard against undefined message handlers - #6902
Conversation
Sorry, something went wrong.
|
What about adding a test based on the code from #6561 (comment)? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@cjihrig I understand. You're probably right. |
Sorry, something went wrong.
|
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}
|
Sorry, something went wrong.
|
@cjihrig I've tried a different fix: santigimeno@7dae5d7. Not too sure about it. What do you think? |
Sorry, something went wrong.
|
@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...catch here. I'm trying to see what the best way forward would be. I think the changes from this PR are still relevant though. |
Sorry, something went wrong.
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)
|
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@santigimeno I came up with a test for this one. It fails for me before this change, and passes after. PTAL |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
The change looks alright to me. LGTM. |
Sorry, something went wrong.
Sorry, something went wrong.
|
CI was green minus what appears to be a Jenkins hiccup on ARM. |
Sorry, something went wrong.
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. Refs: nodejs#6561 PR-URL: nodejs#6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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. Refs: nodejs#6561 PR-URL: nodejs#6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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. Refs: #6561 PR-URL: #6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
|
Yes, please. |
Sorry, something went wrong.
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. Refs: #6561 PR-URL: #6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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. Refs: #6561 PR-URL: #6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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. Refs: #6561 PR-URL: #6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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. Refs: #6561 PR-URL: #6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
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. Refs: #6561 PR-URL: #6902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Checklist
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