Should really not install signal handlers that call process.exit with no way to opt out

[jasonk]

Because threads.js installs it's own signal handlers for SIGINT and SIGTERM and those handlers call process.exit, that means it's impossible to ensure that whatever other cleanup needs to happen when terminated actually happens. Because of this libraries should not generally install signal handlers at all, leaving this up to the application code. This is why the tiny-worker faq mentioned in the code is a FAQ, because it would be unwise for tiny-worker to install those signal handlers itself, the developer using tiny-worker needs to do that at whatever point is appropriate for their application.

I'll attempt to find the time this afternoon to put together a PR, but thought I should at least mention this problem in case anyone else is having issues with cleanup not happening. Personally, my opinion is that a library like this shouldn't be adding signal handlers at all, though you could provide a method that the developer could call to opt-in to this feature. If you intend to keep the signal handlers enabled by default though, we need at a minimum some way to opt out of that.

[andywer]

Hey @jasonk. Thanks a lot for raising the issue!

To be precise, we only set those signal handlers in the worker threads, never in the main thread. Sure, without a way to opt-out that is still a rather bad design choice…

[andywer]

IDK if this is the most elegant way to solve it, but my first idea was something alone these lines…

// worker.js
import { expose, preventDefaultSignalHandlers } from "threads/worker";

preventDefaultSignalHandlers();

// …

Maybe not the prettiest solution, but it would allow us to keep the default behavior and offer you a way out.

[clhuang]

It seems you do terminate the master process: https://github.com/andywer/threads.js/blob/master/src/master/implementation.node.ts#L142

We encountered an unfortunate interaction where our SIGTERM handlers basically were nullified by the presence of terminateWorkersAndMaster; we weren't even using threads directly, rather it was a transitive dependency of https://github.com/geotiffjs/geotiff.js/

[Huararanga]

I agree with clhuang, this lib should nothing to do with signal handlers. At least not implicitly.

Our project is working under kubernetes and we are using customized gracefull shutdown (some logging, shut down some systems, other cleanup) to be able to corectly disable old pods and start new pods.

So we would like to just add pool.settle/complet/terminate commands to our customized gracefull shutdown.

Because of implicit signal handlers we need to call process.removeAllListeners('SIGTERM') to fix the behaviour, which is not very clean.

I aggre to implement preventDefaultSignalHandlers right now.

But I believe the

process.on("SIGINT", () => terminateWorkersAndMaster())
process.on("SIGTERM", () => terminateWorkersAndMaster())

should not to be implicitly called in next major release..

[CMCDragonkai]

I just hit this error. I was scratching my head why my signal handlers were terminating before finishing its asynchronous work. Modifying signal handling in this library is a very bad idea. I had to trace through my dependencies to find it was the import of Transfer that caused my problem and which led me to this issue.

[andywer]

Yeah, maybe we should just revert #198 and publish it as a new major version…

[gabegorelick]

This is still an issue even in the latest version (v1.7.0). Simply loading the threads module is enough to get the signal handlers that call process.exit in the main (non-worker) processes.

[Senney]

Adding my two cents here, this caused an outage for us as it shut down one of our NodeJS tasks before its clean shutdown could be performed. Not super proud of it, but we're adding this workaround to help us get by.

const removeThreadJsExitListeners = (): void => {
  const sigtermHandlers = process.listeners('SIGTERM');
  const sigintHandlers = process.listeners('SIGINT');

  const removeThreadJsListener = (signal: string, handlerFn: NodeJS.SignalsListener): void => {
    // threads.js has a handler which calls the method below
    // and kills the parent thread.
    const offendingHandlerContent = 'terminateWorkersAndMaster';

    if (handlerFn.toString().includes(offendingHandlerContent)) {
      process.off(signal, handlerFn);
    }
  };

  sigtermHandlers.forEach((listener) => removeThreadJsListener('SIGTERM', listener));
  sigintHandlers.forEach((listener) => removeThreadJsListener('SIGINT', listener));
};

If you choose to go down this road, make sure you've got your own clean-shutdown logic which handles the termination of the any threads.