From d59917b2a359bf72f12a57ed9a32a3841720b608 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Fri, 20 May 2016 19:10:48 -0400 Subject: [PATCH 1/4] child_process: emit IPC messages on next tick 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. Fixes: https://github.com/nodejs/node/issues/6561 PR-URL: https://github.com/nodejs/node/pull/6909 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Santiago Gimeno --- lib/internal/child_process.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 73c641c116d24e..789c29ef777042 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -715,7 +715,9 @@ function handleMessage(target, message, handle) { message.cmd.slice(0, INTERNAL_PREFIX.length) === INTERNAL_PREFIX) { eventName = 'internalMessage'; } - target.emit(eventName, message, handle); + process.nextTick(() => { + target.emit(eventName, message, handle); + }); } function nop() { } From dd21bd9f01de7044ed50e4b03b7dd349dbc7ee4e Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Sat, 21 May 2016 15:54:30 +0200 Subject: [PATCH 2/4] test: verify IPC messages are emitted on next tick The test in this commit runs correctly if IPC messages are properly consumed and emitted. Otherwise, the test times out. Fixes: https://github.com/nodejs/node/issues/6561 PR-URL: https://github.com/nodejs/node/pull/6909 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- test/parallel/test-cluster-ipc-throw.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 test/parallel/test-cluster-ipc-throw.js diff --git a/test/parallel/test-cluster-ipc-throw.js b/test/parallel/test-cluster-ipc-throw.js new file mode 100644 index 00000000000000..f2e7f2822c7315 --- /dev/null +++ b/test/parallel/test-cluster-ipc-throw.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); +const cluster = require('cluster'); + +cluster.schedulingPolicy = cluster.SCHED_RR; + +const server = http.createServer(); + +if (cluster.isMaster) { + server.listen(common.PORT); + const worker = cluster.fork(); + worker.on('exit', common.mustCall(() => { + server.close(); + })); +} else { + process.on('uncaughtException', common.mustCall((e) => {})); + server.listen(common.PORT); + server.on('error', common.mustCall((e) => { + cluster.worker.disconnect(); + throw e; + })); +} From aadfe6c24900688fa7dfb9760451cdfa5f9d1fcc Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 23 May 2016 10:55:48 -0400 Subject: [PATCH 3/4] cluster: close ownerless handles on disconnect() When a worker is disconnecting, it shuts down all of the handles it is waiting on. It is possible that a handle does not have an owner, which causes a crash. This commit closes such handles without accessing the missing owner. Fixes: https://github.com/nodejs/node/issues/6561 PR-URL: https://github.com/nodejs/node/pull/6909 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Santiago Gimeno --- lib/cluster.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/cluster.js b/lib/cluster.js index d60366aaf179c8..175645f713f701 100644 --- 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(); From f0a07d956e9a3cda019ead677efab4c8c4c09e9b Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Mon, 23 May 2016 20:04:24 +0200 Subject: [PATCH 4/4] test: test cluster worker disconnection on error This test checks that ownerless cluster worker handles are closed correctly on disconnection. Fixes: https://github.com/nodejs/node/issues/6561 PR-URL: https://github.com/nodejs/node/pull/6909 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- ...test-cluster-worker-disconnect-on-error.js | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/parallel/test-cluster-worker-disconnect-on-error.js diff --git a/test/parallel/test-cluster-worker-disconnect-on-error.js b/test/parallel/test-cluster-worker-disconnect-on-error.js new file mode 100644 index 00000000000000..e52f7ddee60993 --- /dev/null +++ b/test/parallel/test-cluster-worker-disconnect-on-error.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); +const cluster = require('cluster'); + +cluster.schedulingPolicy = cluster.SCHED_NONE; + +const server = http.createServer(); +if (cluster.isMaster) { + server.listen(common.PORT); + const worker = cluster.fork(); + worker.on('exit', common.mustCall(() => { + server.close(); + })); +} else { + server.listen(common.PORT); + server.on('error', common.mustCall((e) => { + cluster.worker.disconnect(); + })); +}