Should our strategy for handling KeyboardInterrupt be usable for other signals too? #134

Activity

1. Badg commented on Apr 21, 2017

   

   Badg

   on Apr 21, 2017

   More actions

   I would be interested in seeing the equivalent for Windows as well, though admittedly I have next to no recollection how Windows notifies running processes of imminent death, so it may very well be more trouble than it's worth. But that's something missing from hypergolix, which may eventually transition over to Trio for reliability reasons.
2. njsmith commented on Apr 23, 2017

   

   njsmith

   on Apr 23, 2017

   MemberAuthor

   More actions

   Ah, Windows is an interesting point. AFAICT Windows doesn't have any equivalent of catching SIGTERM; for ordinary process termination there's just TerminateProcess, and that's the equivalent of SIGKILL. But, they do have "services", which are the equivalent of Unix daemons, and the service API does have a way for a "service manager" (their equivalent or runit or supervisord or whatever) to send a request to a service to exit cleanly.

   So this is making me think that maybe what we really want here is some helpful high-level APIs for writing daemon-like programs, that make it easy to handle the three main cases of run in foreground, run daemonized, run as windows service, and abstract over things like SIGTERM versus SERVICE_CONTROL_STOP.

   Prior art:

   • https://twistedmatrix.com/trac/ticket/5705
   • https://twistedmatrix.com/documents/current/core/howto/application.html
3. dstufft commented on Jul 20, 2018

   

   dstufft

   on Jul 20, 2018

   More actions

   For what it's worth, I think a lot of things fire SIGTERM for a graceful shutdown, and SIGKILL for a "kill it now, with fire". A quick look around tells me Kubernetes and systemd both fire SIGTERM, wait a bit, then fire SIGKILL.

   👍React with 👍4Reacted by Ivan Shapovalov, Ryan Hiebert, Jim Brännlund and Sascha Desch
4. oremanj

   changed the title [-]clean shutdown on SIGTERM[/-] [+]Should our strategy for handling KeyboardInterrupt be usable for other signals too?[/+] on Mar 12, 2019
5. mehaase commented on Jun 27, 2019

   

   mehaase

   on Jun 27, 2019

   Last edited by mehaase

   More actions

   Docker also sends SIGTERM when stopping a container, waits for a grace period, then sends SIGKILL if needed.

   ...there's also an argument that trio's default behavior should be to raise trio.TerminatedError or something on SIGTERM

   This is my favored approach. But I understand if others think this is too magical.

   having the Python-level handler keep track of how often it's been called, and if it gets called more than some threshold, switch to just killing the process (because it appears to be wedged), maybe after printing a message. This is probably only really useful for SIGINT though, since SIGTERM isn't generally used interactively...

   I like this idea, especially if the first SIGINT is restricted to checkpoints. Then the nth SIGINT (I would expect n=2) can be used to break out of loops that don't have any checkpoints. Best of both worlds.
6. Tronic commented on Sep 9, 2019

   

   Tronic

   on Sep 9, 2019

   Contributor

   More actions

   Anything other than the process just disappearing would be better. A small signal receiver block in "application running" state is handy but if a SIGTERM appears anywhere else, finally and __exit__ don't get run.

   Wrapping the entire application into a signal receiver is problematic because then it should be guaranteed that at any phase during startup and shutdown the signals actually get received. Otherwise the application may become very hard to terminate.
7. njsmith commented on Sep 12, 2019

   

   njsmith

   on Sep 12, 2019

   MemberAuthor

   More actions

   Wrapping the entire application into a signal receiver is problematic because then it should be guaranteed that at any phase during startup and shutdown the signals actually get received. Otherwise the application may become very hard to terminate.

   I'm not sure I'm understanding. During startup or shutdown, when the signal receiver isn't running, then you get the default signal behavior, of killing the process. Which is probably what you want. (If you haven't started up yet, or have already shut down, then there's no need to do any more cleanup.)

   There is a race condition around unregistering the signal handler, where a signal could arrive just while you're in the middle of unregistering it and potentially get lost. I believe that Trio is the only Python async library that doesn't lose signals in this case. We take some special care, where if a signal arrives during the critical period, we wait until we've finished restoring the old signal handlers and then re-deliver it, so that it acts like it arrived a few milliseconds later:

   trio/trio/_signals.py

   Lines 77 to 93 in 6df291b

   	def _redeliver_remaining(self):
   	# First make sure that any signals still in the delivery pipeline will
   	# get redelivered
   	self._closed = True
   	# And then redeliver any that are sitting in pending. This is done
   	# using a weird recursive construct to make sure we process everything
   	# even if some of the handlers raise exceptions.
   	def deliver_next():
   	if self._pending:
   	signum, _ = self._pending.popitem(last=False)
   	try:
   	signal_raise(signum)
   	finally:
   	deliver_next()
   	deliver_next()
8. Tronic commented on Sep 12, 2019

   

   Tronic

   on Sep 12, 2019

   Contributor

   More actions

   My first attempt was this kind of construct:

   async def main():
       await application_init()
       with open_nursery() as nursery:
           nursery.start_soon(application_run)
           with open_signal_receiver(SIGINT, SIGTERM) as sigiter:
               async for s in sigiter:
                   nursery.cancel_scope.cancel()
                   break
       await application_shutdown()

   I intended for KeyboardInterrupt style handling of interrupts during application_init and application_shutdown -- but SIGTERM just terminates the process. Moving the signal receiver with block outside everything else makes the application ignore signals during init and shutdown (because no-one is receiving), making it impossible to terminate.

   The current revision:

   async def monitor_signals(signal_receiver, cancel_scopes):
       async for s in signal_receiver:
           cancel_scopes.pop().cancel()
   
   def handle_signals(fn):
       @wraps(fn)
       async def wrapper(*args, **kwargs):
           async with trio.open_nursery() as signal_nursery:
               cancel_scopes = [signal_nursery.cancel_scope]
               with trio.open_signal_receiver(SIGINT, SIGTERM) as signal_receiver:
                   signal_nursery.start_soon(monitor_signals, signal_receiver, cancel_scopes)
                   await fn(*args, **kwargs, cancel_scopes=cancel_scopes)
                   signal_nursery.cancel_scope.cancel()
       return wrapper
   
   @handle_signals
   async def main(cancel_scopes):
       await application_init()
       async with trio.open_nursery() as app_nursery:
           cancel_scopes.append(app_nursery.cancel_scope)
           async with trio.open_nursery() as acceptor:
               cancel_scopes.append(acceptor.cancel_scope)
               await application_run(app_nursery)  # Spawns workers into app_nursery
           cancel_scopes.pop()  # acceptor
           # awaiting for workers to finish
       cancel_scopes.pop()  # app_nursery
       await application_shutdown()

   This is somewhat more refined in attempting to support multiple levels of graceful termination, and obscures the main application logic a lot less with signal handling.

   The idea is to cancel the innermost scope (that has added itself to cancel_scopes) on each interrupt. The first one cancels acceptor but allows gracefully finishing any running tasks -- unless that takes too long and another signal is received, in which case application_shutdown is still attempted, and so on, until finally cancel_scope is canceled and the signal handler code should exit before monitor_signals crashes trying to pop from empty list.

   I am not entirely happy with this construct either, mainly because cancel_scopes needs to be passed around and fiddled with manually (I did consider my own context managers or a wrapper around open_nursery instead of append/pop -- but they'd still be very clumsy/invasive).

   Additionally, there is nothing to be done if the application code is stuck somewhere without scheduling points. The signal receiver never runs and the interrupts sit in Trio's internal queue for eternity.

   The ideal would be:

   1. Raise KeyboardInterrupt for all signals that would otherwise instantly terminate the process (i.e. have SIG_DFL handler and no receiver).
   2. Whenever a signal is received and being appended to receiver queue, and there are signals older than one second in that queue, raise KeyboardInterrupt in the frame that was running.

   Part 1 provides useful defaults for all applications that rely on context managers or try-finally style shutdown. Part 2 takes care of situations where the signal receiver loop cannot run in a timely manner.
9. intelfx commented on Feb 24, 2020

   

   intelfx

   on Feb 24, 2020

   Last edited by intelfx

   More actions

   Please pardon my ignorance — but why won't a simple solution suffice: how about a trio.run(..., extra_signals=[SIGTERM], extra_signals_exception=SomethingBaseException)? Then it's just a matter of handling [SIGINT] + extra_signals where you would only catch [SIGINT] before, and a condition on which exception to raise.

   The restrict_keyboard_interrupt_to_checkpoints can be then renamed to something like restrict_signals_to_checkpoints, with the old name accepted as a compatibility alias.

   ---

   As of now, I just did this:

   def main():
       <...>
       signal.signal(signal.SIGTERM, lambda sig, frame: signal.raise_signal(signal.SIGINT))
       <...>
   

   ...that is, I just convert SIGTERMs to SIGINTs and let internal Trio magic deal with the signal in its fancy race-free way. Ugly, but as close to a proper solution as I can imagine.
   Did I miss anything?

   👍React with 👍1Reacted by Yutian Li

   ❤️React with ❤️1Reacted by Yutian Li

   🚀React with 🚀1Reacted by Yutian Li