From e6878a66eb6a5f8044abd23a03d0385cdb9398cd Mon Sep 17 00:00:00 2001 From: Bar Admoni Date: Sat, 11 Jul 2020 16:45:48 +0300 Subject: [PATCH] cluster: make kill to be just process.kill Make `Worker.prototype.kill` to be just process.kill without preforming graceful disconnect beforehand. Refs: https://github.com/nodejs/node/issues/33715 --- README.md | 2 + doc/api/cluster.md | 10 +- lib/internal/cluster/master.js | 11 +-- .../test-cluster-worker-kill-signal.js | 96 +++++++++++++++++++ 4 files changed, 103 insertions(+), 16 deletions(-) create mode 100644 test/parallel/test-cluster-worker-kill-signal.js diff --git a/README.md b/README.md index fda0fc98ea2340..35cb58c14cf7d3 100644 --- a/README.md +++ b/README.md @@ -437,6 +437,8 @@ For information about the governance of the Node.js project, see **Yosuke Furukawa** <yosuke.furukawa@gmail.com> * [ZYSzys](https://github.com/ZYSzys) - **Yongsheng Zhang** <zyszys98@gmail.com> (he/him) +* [baradm100](https://github.com/baradm100) - +**Bar Admoni** <admoni.bar1@gmail.com> (he/him) ### Collaborator Emeriti diff --git a/doc/api/cluster.md b/doc/api/cluster.md index 1ed5e38670fb3c..07cf615ad90a94 100644 --- a/doc/api/cluster.md +++ b/doc/api/cluster.md @@ -427,14 +427,10 @@ added: v0.9.12 This function will kill the worker. In the master, it does this by disconnecting the `worker.process`, and once disconnected, killing with `signal`. In the -worker, it does it by disconnecting the channel, and then exiting with code `0`. +worker, it does it by killing the process with `signal`. -Because `kill()` attempts to gracefully disconnect the worker process, it is -susceptible to waiting indefinitely for the disconnect to complete. For example, -if the worker enters an infinite loop, a graceful disconnect will never occur. -If the graceful disconnect behavior is not needed, use `worker.process.kill()`. - -Causes `.exitedAfterDisconnect` to be set. +The `kill()` function kills the worker process without waiting for a graceful +disconnect, it have the same behavior of `worker.process.kill()`. This method is aliased as `worker.destroy()` for backwards compatibility. diff --git a/lib/internal/cluster/master.js b/lib/internal/cluster/master.js index 9e2c7cbecb9963..a14adc6723216f 100644 --- a/lib/internal/cluster/master.js +++ b/lib/internal/cluster/master.js @@ -360,14 +360,7 @@ Worker.prototype.disconnect = function() { Worker.prototype.destroy = function(signo) { const proc = this.process; + const signal = signo || 'SIGTERM'; - signo = signo || 'SIGTERM'; - - if (this.isConnected()) { - this.once('disconnect', () => proc.kill(signo)); - this.disconnect(); - return; - } - - proc.kill(signo); + proc.kill(signal); }; diff --git a/test/parallel/test-cluster-worker-kill-signal.js b/test/parallel/test-cluster-worker-kill-signal.js new file mode 100644 index 00000000000000..902b62337533b4 --- /dev/null +++ b/test/parallel/test-cluster-worker-kill-signal.js @@ -0,0 +1,96 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; +// test-cluster-worker-kill-signal.js +// verifies that when we're killing a worker using Worker.prototype.kill +// and the worker's process was killed with the given signal (SIGKILL) + + +const common = require('../common'); +const assert = require('assert'); +const cluster = require('cluster'); + +if (cluster.isWorker) { + const http = require('http'); + const server = http.Server(() => { }); + + server.once('listening', common.mustCall(() => { })); + server.listen(0, '127.0.0.1'); + +} else if (cluster.isMaster) { + const KILL_SIGNAL = 'SIGKILL'; + const expected_results = { + worker_emitDisconnect: [1, "the worker did not emit 'disconnect'"], + worker_emitExit: [1, "the worker did not emit 'exit'"], + worker_state: ['disconnected', 'the worker state is incorrect'], + worker_exitedAfter: [false, 'the .exitedAfterDisconnect flag is incorrect'], + worker_died: [true, 'the worker is still running'], + worker_exitCode: [null, 'the worker exited w/ incorrect exitCode'], + worker_signalCode: [KILL_SIGNAL, + 'the worker exited w/ incorrect signalCode'] + }; + const results = { + worker_emitDisconnect: 0, + worker_emitExit: 0 + }; + + + // start worker + const worker = cluster.fork(); + + // When the worker is up and running, kill it + worker.once('listening', common.mustCall(() => { + worker.kill(KILL_SIGNAL); + })); + + // Check worker events and properties + worker.on('disconnect', common.mustCall(() => { + results.worker_emitDisconnect += 1; + results.worker_exitedAfter = worker.exitedAfterDisconnect; + results.worker_state = worker.state; + })); + + // Check that the worker died + worker.once('exit', common.mustCall((exitCode, signalCode) => { + results.worker_exitCode = exitCode; + results.worker_signalCode = signalCode; + results.worker_emitExit += 1; + results.worker_died = !common.isAlive(worker.process.pid); + })); + + process.on('exit', () => { + checkResults(expected_results, results); + }); +} + +// Some helper functions ... + +function checkResults(expected_results, results) { + for (const k in expected_results) { + const actual = results[k]; + const expected = expected_results[k]; + + assert.strictEqual( + actual, expected && expected.length ? expected[0] : expected, + `${expected[1] || ''} [expected: ${expected[0]} / actual: ${actual}]`); + } +}