Recent SIGINT change breaks ipython / cysignals -- affects sagemath

[tornaria]

Starting with 3.0.25 (i.e. after #1538) hitting control-C is not working properly in sagemath.

Background: sagemath uses cysignals so that signals can be handled when running extension code; cysignals installs a custom signal handler to catch signals while running extension code. When a signal is received, it unwinds and raises a python exception. This is used extensively in sagemath.

The issue is not tied with sagemath and can be easily reproduced from ipython as follows:

• Create an extension module with a function that loops forever using cysignals to trap signals.
• Load the module and run the function from ipython
• Hit control-C: works with prompt_toolkit-3.0.24, doesn't work with prompt_toolkit-3.0.25

EXAMPLE

1. Create a file forever.pyx:

from cysignals.signals cimport sig_on, sig_off

def run():
    sig_on()
    while True:
        pass
    sig_off()

2. Compile this module with

$ cythonize -a -i forever.pyx
Compiling /tmp/forever.pyx because it changed.
[...]

After this you'll have an extension module, in my system the file is named forever.cpython-310-x86_64-linux-gnu.so.

3. Run ipython with prompt_toolkit-3.0.24:

$ ipython
Python 3.10.2 (main, Jan 15 2022, 03:11:32) [GCC 10.2.1 20201203]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.0.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import forever

In [2]: forever.run()
^C---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
Input In [2], in <module>
----> 1 forever.run()

File /tmp/forever.pyx:4, in forever.run()
      2 
      3 def run():
----> 4     sig_on()
      5     while True:
      6         pass

KeyboardInterrupt: 

In [3]: 

4. Run ipython with prompt_toolkit-3.0.25:

$ ipython
Python 3.10.2 (main, Jan 15 2022, 03:11:32) [GCC 10.2.1 20201203]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.0.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import forever

In [2]: forever.run()
^C^C^C^C^C^C^C^C^C^C^C^C

5. Sending a SIGINT from another process also doesn't work.

$ kill -INT 14853
[nothing happens]

6. Sending a different signal works:

$ kill -ALRM 14853
[this happens in ipython]
---------------------------------------------------------------------------
AlarmInterrupt                            Traceback (most recent call last)
    [... skipping hidden 1 frame]

Input In [2], in <module>
----> 1 forever.run()

File /opt/sage/sage-git/develop/t/forever.pyx:4, in forever.run()
      3 def run():
----> 4     sig_on()
      5     while True:

AlarmInterrupt: 

[tornaria]

This could be fixed by making handle_sigint default to False. In this way prompt_toolkit keeps working as before and doesn't affect signals unless explicitly requested.

BTW, the argument handle_sigint in run() doesn't seem to be used. Should that be passed to run_async()?

Anyway my patch to change the default for handle_sigint is the obvious one:

--- a/prompt_toolkit/application/application.py	2022-02-11 05:06:37.000000000 -0300
+++ b/prompt_toolkit/application/application.py	2022-02-21 10:43:13.828058001 -0300
@@ -634,7 +634,7 @@
         self,
         pre_run: Optional[Callable[[], None]] = None,
         set_exception_handler: bool = True,
-        handle_sigint: bool = True,
+        handle_sigint: bool = False,
         slow_callback_duration: float = 0.5,
     ) -> _AppResult:
         """
@@ -859,7 +859,7 @@
         self,
         pre_run: Optional[Callable[[], None]] = None,
         set_exception_handler: bool = True,
-        handle_sigint: bool = True,
+        handle_sigint: bool = False,
         in_thread: bool = False,
     ) -> _AppResult:
         """

With this patch added to 3.0.28 everything seems to be back to normal, including handling of Ctrl-C in sagemath.

[tornaria]

The root cause of this issue is that calling loop.add_signal_handler() and later loop.remove_signal_handler() will not restore the sigint signal handler to the previous state, but only to the default python signal handler.

A possible solution to this is to save the sigint handler before add_signal_handler() and restore it after add_signal_handler().

It is not enough to use signal.getssignal and signal.signal since these save only the python-level signal handler but cysignals installs the os-level signal handler (struct sigaction) because the whole point is to be able to break extension code which may not run python's handlers for a long time. Fortunately there is cysignals.pysignals which implements saving and restoring of os-level signal handlers.

The patch below does precisely this. To avoid a hard dependency on cysignals the approach is: if cysignals is available, save and restore both python and os-level signal handlers. If cysignals is not available, save and restore only the python-level signal handlers.

I'd appreciate any comments about this proposal. This issue breaks sagemath in a very annoying way -- the only alternative is to hold prompt_toolkit to 3.0.24 (void linux is currently shipping 3.0.28 and we are wary of patching prompt_toolkit without some feedback from upstream, cf void-linux/void-packages#35730 (comment)).

BTW, I realized that the issue affecting xonsh (for which #1538 was introduced) also affects ipython: sending SIGINT to ipython with prompt_toolkit 3.0.24 will crash ipython but with prompt_toolkit 3.0.28 is ok, so keeping it enabled + the patch below is indeed an improvement.

--- a/prompt_toolkit/application/application.py	2022-02-11 05:06:37.000000000 -0300
+++ b/prompt_toolkit/application/application.py	2022-02-22 15:31:02.248681343 -0300
@@ -98,6 +98,10 @@
 except ImportError:
     import prompt_toolkit.eventloop.dummy_contextvars as contextvars  # type: ignore
 
+try:
+    from cysignals import pysignals
+except ImportError:
+    from . import dummy_pysignals as pysignals
 
 __all__ = [
     "Application",
@@ -805,6 +809,9 @@
                 loop = get_event_loop()
 
                 if handle_sigint:
+                    # save sigint handlers
+                    _sigint = signal.getsignal(signal.SIGINT)
+                    _sigint_os = pysignals.getossignal(signal.SIGINT)
                     loop.add_signal_handler(
                         signal.SIGINT,
                         lambda *_: loop.call_soon_threadsafe(
@@ -843,6 +850,8 @@
 
                     if handle_sigint:
                         loop.remove_signal_handler(signal.SIGINT)
+                        # restore python and os-level signal handlers
+                        pysignals.setsignal(signal.SIGINT, _sigint, _sigint_os)
 
                     # Reset slow_callback_duration.
                     loop.slow_callback_duration = original_slow_callback_duration
--- a/prompt_toolkit/application/dummy_pysignals.py	1969-12-31 21:00:00.000000000 -0300
+++ b/prompt_toolkit/application/dummy_pysignals.py	2022-02-22 15:31:26.432386296 -0300
@@ -0,0 +1,10 @@
+# fallback in case cysignals is not available
+# these functions don't save the os level handlers
+
+import signal
+
+def getossignal(sig):
+    return None
+
+def setsignal(sig, action, osaction=None):
+    return signal.signal(sig, action)

[jonathanslenders]

Hi @tornaria,

Thanks for the feedback, and sorry for the delay.
I've just been looking into this, and a couple of thoughts:

• prompt_toolkit has to set signal handlers. Not setting them results in various issues due to cleanup not happening correctly and leaving the Application in a broken state. See: [WIP] Use prompt_toolkit's built-in cursorshape support. ipython/ipython#13567 (comment)
• I'm not really comfortable with the idea of having prompt_toolkit depend on an external library for this. Do you know other popular Python packages that depend on cysignals?

When you say "because the whole point is to be able to break extension code which may not run python's handlers for a long time", does it mean that this can break out of literally any extension code and have an exception propagate? How do you know whether or not this extension holds a lock somewhere? Would this not cause any deadlocks at some point, due to code trying to acquire a lock that was not released earlier to due this signal handling?

[mkoeppe]

This issue breaks sagemath in a very annoying way -- the only alternative is to hold prompt_toolkit to 3.0.24

Now that ipython requires prompt-toolkit >= 3.0.30, this workaround is no longer feasible.

[mkoeppe]

• I'm not really comfortable with the idea of having prompt_toolkit depend on an external library for this.

@jonathanslenders With @tornaria's patch, there wouldn't be a dependency on cysignals; it would just use cysignals if it is installed.

[tornaria]

* I'm not really comfortable with the idea of having prompt_toolkit depend on an external library for this. Do you know other popular Python packages that depend on cysignals?

Would you feel more comfortable if I try to implement the same using ctypes and signal which are standard python modules? Note that signal alone is not enough because it only allows one to save and restore the python level handler, but not the os level handler.

The only thing needed here is that the set_handle_sigint() context completely restores the signal handler, not just the python signal handler but also the os-level signal handler.

When you say "because the whole point is to be able to break extension code which may not run python's handlers for a long time", does it mean that this can break out of literally any extension code and have an exception propagate? How do you know whether or not this extension holds a lock somewhere? Would this not cause any deadlocks at some point, due to code trying to acquire a lock that was not released earlier to due this signal handling?

This is not automatic: signals only work between matching calls to _sig_on() and _sig_off(). It is the responsibility of the programmer to make sure it is safe to interrupt the code. In practice it works quite well, so long as the signal handler is not messed up. It's not a problem for us if prompt_toolkit installs a custom handler, as long as it restores it back when it's done.

[tornaria]

@jonathanslenders : Here's a new proposal that uses ctypes and python stable API. No need for any external module. I've been using this on 3.0.38 for a while.

--- a/src/prompt_toolkit/application/application.py
+++ b/src/prompt_toolkit/application/application.py
@@ -17,6 +17,7 @@ from asyncio import (
     sleep,
 )
 from contextlib import ExitStack, contextmanager
+from ctypes import pythonapi, c_int, c_void_p
 from subprocess import Popen
 from traceback import format_tb
 from typing import (
@@ -100,6 +101,18 @@ _SIGWINCH = getattr(signal, "SIGWINCH", None)
 _SIGTSTP = getattr(signal, "SIGTSTP", None)
 
 
+# The following functions are part of the stable ABI since python 3.2
+# See: https://docs.python.org/3/c-api/sys.html#c.PyOS_getsig
+
+# PyOS_sighandler_t PyOS_getsig(int i)
+pythonapi.PyOS_getsig.restype = c_void_p
+pythonapi.PyOS_getsig.argtypes = c_int,
+
+# PyOS_sighandler_t PyOS_setsig(int i, PyOS_sighandler_t h)
+pythonapi.PyOS_setsig.restype = c_void_p
+pythonapi.PyOS_setsig.argtypes = c_int, c_void_p,
+
+
 class Application(Generic[_AppResult]):
     """
     The main Application class!
@@ -794,6 +807,10 @@ class Application(Generic[_AppResult]):
         @contextmanager
         def set_handle_sigint(loop: AbstractEventLoop) -> Iterator[None]:
             if handle_sigint:
+                # save sigint handlers (python and os level)
+                # See: https://github.com/prompt-toolkit/python-prompt-toolkit/issues/1576
+                sigint = signal.getsignal(signal.SIGINT)
+                sigint_os = pythonapi.PyOS_getsig(signal.SIGINT)
                 loop.add_signal_handler(
                     signal.SIGINT,
                     lambda *_: loop.call_soon_threadsafe(
@@ -804,6 +821,8 @@ class Application(Generic[_AppResult]):
                     yield
                 finally:
                     loop.remove_signal_handler(signal.SIGINT)
+                    signal.signal(signal.SIGINT, sigint)
+                    pythonapi.PyOS_setsig(signal.SIGINT, sigint_os)
             else:
                 yield
 

[tornaria]

And here is how to reproduce the issue in ipython, without any reference to sagemath or cysignals. The example below installs a custom sigint handler, runs an infinite loop, and then send ^C.

1. With unpatched 3.0.38, the custom handler is not called:

$ ipython
Python 3.11.3 (main, Apr  6 2023, 22:56:38) [GCC 12.2.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.13.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: def h(x,y): print(f"Custom handler: {x = }, {y = }"); raise KeyboardInterrupt

In [2]: import signal ; signal.signal(signal.SIGINT, h)
Out[2]: <function _signal.default_int_handler(signalnum, frame, /)>

In [3]: while True: pass
^C---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
Cell In[3], line 1
----> 1 while True: pass

KeyboardInterrupt: 

In [4]: 

2. With 3.0.38 with my patch from previous comment, the custom signal handler is called:

$ ipython
Python 3.11.3 (main, Apr  6 2023, 22:56:38) [GCC 12.2.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.13.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: def h(x,y): print(f"Custom handler: {x = }, {y = }"); raise KeyboardInterrupt

In [2]: import signal ; signal.signal(signal.SIGINT, h)
Out[2]: <function _signal.default_int_handler(signalnum, frame, /)>

In [3]: while True: pass
^CCustom handler: x = 2, y = <frame at 0x7fe0c66763b0, file '<ipython-input-3-b16dc615ea65>', line 1, code <module>>
---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
Cell In[3], line 1
----> 1 while True: pass

Cell In[1], line 1, in h(x, y)
----> 1 def h(x,y): print(f"Custom handler: {x = }, {y = }"); raise KeyboardInterrupt

KeyboardInterrupt: 

In [4]: