thefrontside / effection

Structured concurrency and effects for JavaScript
https://frontside.com/effection
MIT License
592 stars 25 forks source link

Listen for SIGTERM too #874

Closed wms closed 11 months ago

wms commented 11 months ago

Install interrupt handler for SIGTERM events, as commonly used by watchers/supervisors.

@TODO: the interrupt fn always reports that the signal received was SIGINT even if SIGTERM was used. Parameterize interrupt to correct report received signal?

Motivation

The dev-mode supervision tool tsx sends a SIGTERM to the child process when a change is detected; it would be nice for Effection to catch these too.

Approach

Simply install the interrupt handler in response to SIGTERM events just like SIGINT

Alternate Designs

I've currently got the following baked into my Node app, which works, but would be nice to omit:

/**
 * Shuts down Effection when SIGTERM is received.
 * @TODO Raise PR to bake this into Effection's `main` implementation
 */
function* useSigTerm() {
  const { run } = yield* useScope();
  const handler = () =>
    run(function* () {
      yield* exit(130);
    });

  process.on("SIGTERM", handler);

  yield* ensure(() => {
    process.off("SIGTERM", handler);
  });
}

await main(function* () {
  yield* useSigTerm();
  /* ... */

Possible Drawbacks or Risks

Maybe downstream users have custom behaviour bound to SIGTERM.

TODOs and Open Questions

cowboyd commented 11 months ago

@wms I think this totally makes sense, and in fact, this was the behavior in v2 https://github.com/thefrontside/effection/blob/v2/packages/main/src/node.ts#L41-L42

So I think this would be an improvement for sure. Before merging there are a couple of issues that would have to be addressed.

  1. You're right, we would need to parameterize the interrupt to indicate that it was sigterm
  2. We need a test case. I'd imagine you could replicate this as a start: https://github.com/thefrontside/effection/blob/types-for-all/test/main.test.ts#L5-L22

I really like your work around by the way.!

wms commented 11 months ago

Thank you sir, but credit goes to you folks in making the API clean enough to make the workaround slot in so nicely and not feel at all hacky. I'll put in a test case + parameterized interrupt handler.

wms commented 11 months ago

@cowboyd Branch updated to include separate interrupts for each signal and test coverage.

cowboyd commented 11 months ago

@wms This looks great, and thanks for taking taking the time to contribute! This should be released some time next week.