Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve Process#on_interrupt #13654

Closed
wants to merge 11 commits into from

Conversation

stakach
Copy link
Contributor

@stakach stakach commented Jul 12, 2023

This fixes some inconsistences between the platforms

  • previously the unix implementation was not ignoring the interrupt whilst the windows version is.

Also improves usefulness

  • as on unix we typically want to handle INT, HUP and TERM more or less equivalently
  • The windows implementation misses some other useful events like the user closing the console window

effectively this function becomes the termination requested callback
the previous implementation meant that you would still have to implement quite a bit of system specific code to handle the other signals

Example of the state of this function before this pull

@@ -103,7 +103,7 @@ struct Crystal::System::Process
def self.on_interrupt(&@@interrupt_handler : ->) : Nil
restore_interrupts!
@@win32_interrupt_handler = handler = LibC::PHANDLER_ROUTINE.new do |event_type|
next 0 unless event_type.in?(LibC::CTRL_C_EVENT, LibC::CTRL_BREAK_EVENT)
next 0 unless event_type.in?(LibC::CTRL_C_EVENT, LibC::CTRL_BREAK_EVENT, LibC::CTRL_CLOSE_EVENT, LibC::CTRL_LOGOFF_EVENT, LibC::CTRL_SHUTDOWN_EVENT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest moving these into a constant for readability purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably remove the line entirely as I'm pretty sure these are all the possible events

@straight-shoota
Copy link
Member

straight-shoota commented Jul 13, 2023

The original idea of Process.on_interrupt is really specific to an interrupt signal, that is SIGINT and similar on other platforms (ref. #12224: "an interrupt is defined to be whatever Ctrl+C in a terminal produces").
Now I understand the use case of handlers for other termination signals. But I'm worried if this should all be combined into .on_interrupt. Maybe there should be a similar Process.on_terminate instead? Not sure.
At the very least, I think the handler would need some information about the type of signal that was received.

@stakach
Copy link
Contributor Author

stakach commented Jul 13, 2023

Yeah I understand what you're saying but a cross platform way to handle just ctrl-c is not very useful.
In my experience, it's rare that you care why the process has been interrupted.

Like what's the difference between pressing ctrl-c and closing the terminal window from a users perspective.
From a nix perspective it's INT and SIGHUP, on windows it's CTRL_C and CTRL_CLOSE

Also if I shutdown my computer or ask docker to stop (SIGTERM) or my ssh session exits I want the app to exit cleanly and it doesn't make sense to have the developer have to deal with all the different cases, on each platform, as the default. Sure if you want some special behaviour do it the hard way, but the cross platform helper should handle all that for you.

I also get that at_exit exists but I'm guessing you can't spawn fibers in that or async wait for say HTTP requests to drain etc

At the very least, I think the handler would need some information about the type of signal that was received

maybe we just pass the constant value? but then it gets very platform specific again and currently won't compile on unix

Process.on_interrupt do |code|
  case code
  when Signal::INT, LibC::CTRL_C_EVENT?
  end
end

unless we alias the window symbols in Signal? so we could do Signal::CTRL_C_EVENT
or we could map the windows events to unix ones for ease of use

something like what ruby does when running on windows

  • Signal::INT => CTRL_C_EVENT
  • Signal::ABRT => CTRL_BREAK_EVENT
  • Signal::HUP => CTRL_CLOSE_EVENT
  • Signal::TERM => CTRL_SHUTDOWN_EVENT
  • CTRL_LOGOFF_EVENT ??

also worth noting that ruby, rust and go lang all only support CTRL_C_EVENT on windows without interacting with the windows API and all emulate how you'd setup the handler on nix (i.e. no special handler) which is pretty poor really and possibly why we think we should limit ourselves to just SIGINT?

@stakach
Copy link
Contributor Author

stakach commented Jul 22, 2023

closed in favour of #13694

@stakach stakach closed this Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants