From d7edfe9cae184ea0c90e1bca956ef59988a434a8 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 20 Dec 2020 21:06:54 -0500 Subject: [PATCH 1/5] Signal handling: Factor out common signal handling code --- src/master/implementation.node.ts | 41 ++++++++++++++----------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/master/implementation.node.ts b/src/master/implementation.node.ts index bf5a6ddb..2bed7607 100644 --- a/src/master/implementation.node.ts +++ b/src/master/implementation.node.ts @@ -27,6 +27,19 @@ let tsNodeAvailable: boolean | undefined export const defaultPoolSize = cpus().length +interface Terminable { + terminate(this: Terminable): any +} + +// Terminates the workers, empties the workers array, and exits. +const onSignal = (workers: Terminable[], signal: string) => { + Promise.all(workers.map(worker => worker.terminate())).then( + () => process.exit(0), + () => process.exit(1), + ) + workers.length = 0 +} + function detectTsNode() { if (typeof __non_webpack_require__ === "function") { // Webpack build: => No ts-node required or possible @@ -98,7 +111,7 @@ function initWorkerThreadsWorker(): ImplementationExport { ? __non_webpack_require__("worker_threads").Worker : eval("require")("worker_threads").Worker - let allWorkers: Array = [] + const allWorkers: Array = [] class Worker extends NativeWorker { private mappedEventListeners: WeakMap @@ -139,18 +152,9 @@ function initWorkerThreadsWorker(): ImplementationExport { } } - const terminateWorkersAndMaster = () => { - // we should terminate all workers and then gracefully shutdown self process - Promise.all(allWorkers.map(worker => worker.terminate())).then( - () => process.exit(0), - () => process.exit(1), - ) - allWorkers = [] - } - // Take care to not leave orphaned processes behind. See #147. - process.on("SIGINT", () => terminateWorkersAndMaster()) - process.on("SIGTERM", () => terminateWorkersAndMaster()) + process.on("SIGINT", (signal) => onSignal(allWorkers, signal)) + process.on("SIGTERM", (signal) => onSignal(allWorkers, signal)) class BlobWorker extends Worker { constructor(blob: Uint8Array, options?: ThreadsWorkerOptions) { @@ -219,19 +223,10 @@ function initTinyWorker(): ImplementationExport { } } - const terminateWorkersAndMaster = () => { - // we should terminate all workers and then gracefully shutdown self process - Promise.all(allWorkers.map(worker => worker.terminate())).then( - () => process.exit(0), - () => process.exit(1), - ) - allWorkers = [] - } - // Take care to not leave orphaned processes behind // See - process.on("SIGINT", () => terminateWorkersAndMaster()) - process.on("SIGTERM", () => terminateWorkersAndMaster()) + process.on("SIGINT", (signal) => onSignal(allWorkers, signal)) + process.on("SIGTERM", (signal) => onSignal(allWorkers, signal)) class BlobWorker extends Worker { constructor(blob: Uint8Array, options?: ThreadsWorkerOptions) { From f8c60879131f14bdaa84a9c07f3b887d4119b213 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 20 Dec 2020 21:08:16 -0500 Subject: [PATCH 2/5] Signal handling: Always exit non-zero The default signal handler always exits non-zero so the signal listener installed by threads.js should too. --- src/master/implementation.node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/master/implementation.node.ts b/src/master/implementation.node.ts index 2bed7607..cd8097f9 100644 --- a/src/master/implementation.node.ts +++ b/src/master/implementation.node.ts @@ -34,7 +34,7 @@ interface Terminable { // Terminates the workers, empties the workers array, and exits. const onSignal = (workers: Terminable[], signal: string) => { Promise.all(workers.map(worker => worker.terminate())).then( - () => process.exit(0), + () => process.exit(1), () => process.exit(1), ) workers.length = 0 From 78772486281f3cfd0548b6a6a9cb1170bce06841 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 20 Dec 2020 21:12:00 -0500 Subject: [PATCH 3/5] Signal handling: Allow worker.terminate() to be synchronous This also suppresses any synchronous exceptions thrown by `worker.terminate()`. --- src/master/implementation.node.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/master/implementation.node.ts b/src/master/implementation.node.ts index cd8097f9..e673a2c6 100644 --- a/src/master/implementation.node.ts +++ b/src/master/implementation.node.ts @@ -33,7 +33,10 @@ interface Terminable { // Terminates the workers, empties the workers array, and exits. const onSignal = (workers: Terminable[], signal: string) => { - Promise.all(workers.map(worker => worker.terminate())).then( + // worker.terminate() might return a Promise or might be synchronous. This async helper function + // creates a consistent interface. + const terminate = async (worker: Terminable) => worker.terminate() + Promise.all(workers.map(worker => terminate(worker))).then( () => process.exit(1), () => process.exit(1), ) From 58d04b35c5fafb4a4a0796d44dc9c131b0dcd013 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 20 Dec 2020 21:14:21 -0500 Subject: [PATCH 4/5] Signal handling: Wait for all workers to terminate The promise returned from `Promise.all()` immediately rejects if any of the underlying promises reject, even if some of the other promises have not yet settled. Catch each rejection to give the other promises an opportunity to resolve. `Promise.allSettled()` could be used instead, but that's a relatively new function that was added in Node.js v12.9.0. --- src/master/implementation.node.ts | 5 +---- tslint.json | 1 + 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/master/implementation.node.ts b/src/master/implementation.node.ts index e673a2c6..cc8882b0 100644 --- a/src/master/implementation.node.ts +++ b/src/master/implementation.node.ts @@ -36,10 +36,7 @@ const onSignal = (workers: Terminable[], signal: string) => { // worker.terminate() might return a Promise or might be synchronous. This async helper function // creates a consistent interface. const terminate = async (worker: Terminable) => worker.terminate() - Promise.all(workers.map(worker => terminate(worker))).then( - () => process.exit(1), - () => process.exit(1), - ) + Promise.all(workers.map(worker => terminate(worker).catch(() => {}))).then(() => process.exit(1)) workers.length = 0 } diff --git a/tslint.json b/tslint.json index 1b740a35..8f12f2b1 100644 --- a/tslint.json +++ b/tslint.json @@ -12,6 +12,7 @@ ], "interface-over-type-literal": false, "member-ordering": false, + "no-empty": [true, "allow-empty-functions"], "no-implicit-dependencies": [ true, ["tiny-worker", "worker_threads"] From 6c561d4253b55890bdf3d00b955154f06072e5c4 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 20 Dec 2020 21:21:18 -0500 Subject: [PATCH 5/5] Signal handling: Don't exit if there are other signal listeners --- src/master/implementation.node.ts | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/master/implementation.node.ts b/src/master/implementation.node.ts index cc8882b0..7de6cef9 100644 --- a/src/master/implementation.node.ts +++ b/src/master/implementation.node.ts @@ -31,12 +31,29 @@ interface Terminable { terminate(this: Terminable): any } -// Terminates the workers, empties the workers array, and exits. +// Terminates the workers, empties the workers array, and possibly exits. const onSignal = (workers: Terminable[], signal: string) => { // worker.terminate() might return a Promise or might be synchronous. This async helper function // creates a consistent interface. const terminate = async (worker: Terminable) => worker.terminate() - Promise.all(workers.map(worker => terminate(worker).catch(() => {}))).then(() => process.exit(1)) + Promise.all(workers.map(worker => terminate(worker).catch(() => {}))).then(() => { + // Adding a signal listener suppresses the default signal handling behavior. That default + // behavior must be replicated here, but only if the default behavior isn't intentionally + // suppressed by another signal listener. Unfortunately there is no robust way to determine + // whether the default behavior was intentionally suppressed, so a heuristic is used. (Note: The + // 'exit' event is not suitable for terminating workers because it is not emitted when the + // default signal handler terminates the process.) + if (process.listenerCount(signal) > 1) { + // Assume that one of the other signal listeners will take care of calling process.exit(). + // This assumption breaks down if all of the other listeners are making the same assumption. + return + } + // Right now this is the only signal listener, so assume that this listener is to blame for + // inhibiting the default signal handler. (This assumption fails if the number of listeners + // changes during signal handling. This can happen if a listener was added by process.once().) + // Mimic the default behavior, which is to exit with a non-0 code. + process.exit(1) + }) workers.length = 0 }