Improve Process.on_interrupt for handling more process termination signals

[straight-shoota]

Currently Process.on_interrupt installs a handler for process interrupts. On Unix systems, this corresponds to SIGINT, on Windows its CTRL_C_EVENT and CTRL_BREAK_EVENT.

Handling other termination requests currently requires explicit, platform-specific code, as mentioned in #13694 (comment)

  {% if flag?(:win32) %}
    Process.on_interrupt { shutdown_gracefully }
  {% else %}
    # Detect ctrl-c
    Signal::INT.trap { shutdown_gracefully }

    # Docker containers use the term signal
    Signal::TERM.trap { shutdown_gracefully }
  {% end %}

Having a generic stdlib API would help writing portable code.

On Unix we typically want to handle INT, HUP and TERM more or less equivalently. And the equivalents on Windows are similar.
There should be a simple default mechanism for handling all of these reasons the same way. But for some use cases it may be necessary to differentiate between them,

The reasons can be categorized and mapped as follows (thanks @stakach):

type	unix	windows
interrupted	SIGINT	CTRL_C_EVENT, LibC::CTRL_BREAK_EVENT
terminal disconnected	SIGHUP	CTRL_CLOSE_EVENT
session ended	SIGTERM	CTRL_LOGOFF_EVENT, LibC::CTRL_SHUTDOWN_EVENT

Now we could have a combined handler for all of them, and a means to select different behaviour when it's called. Alternatively, there could be separate handlers, for each one. But then we'd need a simple mechanism to set up a handler (the same one) for all of them.

A tangentially related issue is #13803

[straight-shoota]

#13694 is a proposal to enhance Process.on_interrupt by passing the event type as an input parameter to the proc.
A problem with this approach is backwards compatibility (see #13694 (comment)). Otherwise it seems like a neat and simple extension of the existing API.

[devnote-dev]

Why can't #13694 be implemented as an overload? It would likely mean having two handler types but the current could just be deprecated, and the transition to the new signature is minor.

[straight-shoota]

Unfortunately, it's impossible to overload on the type of a block parameter.

[stakach]

I could modify my pull request to implement on_interrupt as a macro to detect if the block has parameters and effectively overload the function for compatibility

[straight-shoota]

Turning Process.on_interrupt into a macro could indeed help to work around this. However, I'm not very happy about it.
It should really be a method. This could be a temporary feature to allow a smooth migration path.

[straight-shoota]

I'm wondering if it could be possible to accept Proc(X, Y) for a type restriction of Proc(Y). This would require a compiler change, of course. And I'm not sure if it's feasible. But at least theoretically, it should be fine to call a proc that does not expect any input parameters with some input parameters. They would just be ignored.

[stakach]

I've updated the pull to check the arity of the proc and call it appropriately - no macros required

[HertzDevil]

Rethinking about this, the other types of events are, well, not interrupts. The name on_interrupt was chosen chiefly for the platform-independent Ctrl+C. So I am not sure if a single Process.on_interrupt expresses the intent properly anymore.

[stakach]

Rethinking about this, the other types of events are, well, not interrupts.

If you want to get prescriptive, once you throw Windows into the mix are any of the windows events interrupts?

GNU docs group them as "termination signals" https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html
MS calls it a handle routine: https://learn.microsoft.com/en-us/windows/console/handlerroutine

A "process interrupt" seems like a generic enough term to group the behavior though

[HertzDevil]

Ctrl+C is a user-initiated interrupt in a general sense; the spec runner chooses to finish early, other things can choose to ignore it, effectively cancelling the interrupt. With a termination request you cannot cancel the termination in the same manner.

Right now I would do something like this:

class Process
  # handles `INT`, overrides `on_shutdown`
  def self.on_interrupt(& : ->)
  end

  # handles `HUP` and `TERM`
  # also handles `INT`, until `on_interrupt` is called
  def self.on_shutdown(& : ->)
  end

  # ignores `INT` as if by installing an empty handler
  # does not affect `HUP` and `TERM`
  def self.ignore_interrupts!
  end

  # removes the `on_interrupt` handler; if `on_shutdown` had been called before,
  # that handler will handle `INT` again after this
  def self.restore_interrupts!
  end

  # how do we reset the `on_shutdown` handler? do we even need it?
end

[stakach]

I would argue that ctrl-c is user-initiated termination - "I want to stop this process so I can launch another from the same terminal" that can be optionally ignored by the application as the user can always close the terminal window if they really want to stop the process.

At least in my experience this is the more common workflow and use case for the signal

[straight-shoota]

To throw in another data point, SIGHUP is commonly used by daemons as a trigger to reload their configuration. So it's not at all exclusively understood as a termination signal.

The term "interrupt" has different semantics on different levels. A very narrow application would be SIGINT (and maybe equivalent). But it can have a much more general meaning as a disruption of the program's flow for handling some external notification in the sense of https://en.wikipedia.org/wiki/Interrupt
So #on_interrupt should be fitting for the purpose intended here. We don't need to go for a different method name just for correct semantics.

And I think a single callback which receives the type of interrupt as a parameter is better than registering separate callbacks for every type. It makes it easy to group the same responses to different interrupt types.

[HertzDevil]

But CTRL_CLOSE_EVENT does not have an analogous interpretation on Windows; it is a good thing such a platform-specific interpretation doesn't manifest in a purportedly platform-specific API in the standard library. Any such daemons written in Crystal should indeed use Signal::HUP.trap directly, and stdlib should not encourage people to use on_shutdown or on_interrupt as a replacement just because these methods also take care of SIGHUP.

[straight-shoota]

I agree on this specific use case for SIGHUP. My comment was only to mention that the signals we're talking about are not necessarily used with an intention of termination.

[straight-shoota]

Following this discussion I suppose we can agree to move forward with the general API?

There is however another issue with backwards compatibility of program-level semantics.
The following program would continue to build with this change, but the behaviour breaks:

Signal::HUP.trap do
  STDOUT.puts "Signal HUP received"
end
Process.on_interrupt do
  STDOUT.puts "Interrupt"
end

sleep

With current Crystal, the program reacts to SIGHUP with the explicit HUP handler.
After this patch, on_interrupt overrides the explicit HUP handler.

This is a silent breaking change, which is not good. It might even be preferable to have a hard change in the API to have it explicit.
I don't think we need to do that though.

Perhaps the best path forward would really be to add this as a different method with a unique signature and keep Process.on_interrupt as is (probably deprecating it, though).
With agreement on the current naming, maybe Process.on_interrupt!(& : Process::ExitReason ->) could be an option?
It would be a bit confusing with .on_interrupt but it should be fine as that get deprecated.

[m-o-e]

How about making Process.on_interrupt (optionally) raise an Exception
if there is already a handler attached to one of the signals? 🤔

In a nutshell:

def on_interrupt(force : Bool, & : ->)
  ..
  raise SignalHandlerAlreadyAttached.new("") unless Signal::HUP.trap(..).nil? || force
  ..
end

I'm coming from #13803 here - i.e. I think just blindly
overwriting those handlers isn't a great API to begin with.

The above signature would continue to allow that, but force everybody
who upgrades to take a look and make a conscious decision about it.

[stakach]

@straight-shoota would something like this maybe a way forward:

• deprecate on_interrupt
• implement on_terminate with all the signals specified in the GNU docs
• the call to on_terminate will return any existing block specified as ruby does
• maybe we add terminate_handler? which can also return any existing block

I feel like the ruby way of just returning any previously defined callback is probably a good way forward as it's simple.
although having methods to check if there are any defined callbacks implemented would probably be helpful too

In terms of overriding I think that's fine as long as it's documented. Being able to check if there are handlers in place allows devs like @m-o-e to achieve the desired outcome

Signal::HUP.trap do
  STDOUT.puts "Signal HUP received"
end
existing_callback = Signal::HUP.trap_handler?
Process.on_terminate do
  existing_callback.try &.call
  STDOUT.puts "Interrupt"
end

[stakach]

updated the pull request to do the following:

• implements Process.on_terminate(& : Process::ExitReason ->)
• deprecates Process.on_interrupt(&) maintaining its previous signature and behaviour

devs can chain signals on unix using #14126 if they want to handle something independently whilst also using on_terminate