python3-prompt_toolkit: don't handle sigint

[tornaria]

Fixes #35712

See also: prompt-toolkit/python-prompt-toolkit#1576

Testing the changes

• I tested the changes in this PR: YES

[ahesford]

We at least need to wait until upstream comments on your proposed fix before adopting it here. Presumably they merged the change for a reason.

[tornaria]

We at least need to wait until upstream comments on your proposed fix before adopting it here. Presumably they merged the change for a reason.

I understand, you can see their reasons here: prompt-toolkit/python-prompt-toolkit#1537

However let me stress that this breaks sagemath very badly. If I'm in the middle of a computation session and I happen to run a command that takes a very long time, then Ctrl-C is broken and I can't get back to my session (I can Ctrl-Z and kill but then I will loose the session -- a non-obvious workaround is to send SIGALRM instead of SIGTERM which allows to recover).

The alternative to this patch is to revert prompt_toolkit to 3.0.24 until upstream says something. Is there anything requiring a more recent version of prompt_toolkit?

[tornaria]

@ahesford a different approach, explained here: prompt-toolkit/python-prompt-toolkit#1576 (comment). What do you think?

[ahesford]

Between the two options, I suppose I favor just disabling the SIGINT handler by default. Adding cysignals awareness to prompt_toolkit seems too special-purpose a fix; most people will not be using cysignals to add low-level signal handlers. It seems to me that the breakage here is with cysignals, which doesn't seem to be properly overriding the existing handler when it injects its own.

What I would like to see from upstream are some guidance about the following questions:

1. Is disabling SIGINT handling by default, and having xonsh turn it on explicitly, the right course of action?
2. If not, should this be fixed in IPython by providing a configuration option that can override the default SIGINT handler?
3. Is this better addressed (or even addressable at all) in the signal-handling logic of cysignals?

If upstream doesn't respond soon, we can pick up the original patch in this PR.

[tornaria]

Between the two options, I suppose I favor just disabling the SIGINT handler by default. Adding cysignals awareness to prompt_toolkit seems too special-purpose a fix; most people will not be using cysignals to add low-level signal handlers.

My last patch is not adding cysignals awareness. On the contrary, the strategy is to save and restore the sigint handlers so that the signal state is preserved after each run of prompt_toolkit (instead of being reverted to the python default each time prompt_toolkit runs).

This is a generic fix that would work for any program that sets the sigint handler, either just the python sigint handler or the whole os-level sigint handler..

It seems to me that the breakage here is with cysignals, which doesn't seem to be properly overriding the existing handler when it injects its own.

I don't think so. The way cysignals works is, at import time install a sigint handler using sigaction(2) as well as a python sigint handler using signal.signal() from python stdlib, but it expects that these handlers are not overriden -- if they are cysignals will not work anymore.

The way prompt_toolkit implements the sigint handling is: each time the prompt is run, the sigint handler is added (using loop.add_signal_handler()) and when the prompt returns (i.e. the user hit enter to dispatch the command) the sigint handler is removed (using loop.remove_signal_handler()). However, "remove" means "set the handler back to python default" which disables cysignals.

Now prompt_toolkit only needs its own sigint handler while running (i.e. while editing the prompt); and cysignals only needs its own sigint handler while running extension code so it's not a problem that prompt_toolkit installs a custom sigint handler as long as it is later restored. There should be no conflict at all, and nothing there that is specific to cysignals.

The question is: how to save and restore the sigint handler? Python stdlib offers signal.getsignal() (to save) and signal.signal() (to restore), but these are NOT a complete solution as they only save and restore the python signal handlers -- the os-level signal handler will be reverted back to the standard python signal handler which is not good enough for cysignals.

How to save and restore the os-level signal handler? Using sigaction(2), however that needs C code but prompt_toolkit is python only afaict. It turns out cysignals has a submodule called cysignals.pysignals which contains functions to save and restore os-level signal handlers so this seemed like the easiest solution. This adds no dependency on cysignals: if not available, only the python-level signal handlers will be saved and restored.

What I would like to see from upstream are some guidance about the following questions:

1. Is disabling SIGINT handling by default, and having xonsh turn it on explicitly, the right course of action?

I later convinced myself that this is not a good solution. In fact, ipython itself also crashes when receiving a SIGINT. Their fix also fixes ipython so it is a good one (except for not restoring the sigint handler which only affects python programs that change the sigint handler, e.g. those using cysignals).

2. If not, should this be fixed in IPython by providing a configuration option that can override the default SIGINT handler?

No, because ipython itself knows nothing at all about cysignals or sigint handlers, or any signal handler at all. IMHO the only proper solution is that prompt_toolkit restores the sigint handlers that it changes (for a good reason) to their previous values.

3. Is this better addressed (or even addressable at all) in the signal-handling logic of `cysignals`?

I don't think there's any way to fix the signal-handling logic of cysignals: if their sigint handler is removed then cysignals won't catch SIGINT at all, nothing in their logic will fix that.

If upstream doesn't respond soon, we can pick up the original patch in this PR.

As I explained above, I think the second patch is better.

[ahesford]

I misunderstood when prompt_toolkit installed and restored the signal handler; it certainly is problematic that the handler is overridden and restored repeatedly.

The "cysignals awareness" that I mention is your dependence on the cysignals package to save and restore signals set with sigaction. It's not clear to me that upstream would accept an (optional) dependence on cysignals to manage this. Furthermore, if managing signals set with sigaction is the right thing to do, it shouldn't be done only if the user chooses to install cysignals. Thus, either cysignals should become a hard requirement of prompt_toolkit or it should have its own method to save and restore these signals.

We really need some guidance from upstream about whether they intend to care about this problem at all and, if so, how they intend to solve it. I don't want to maintain indefinitely a custom prompt_toolkit patch that pulls in cysignals to manage these signals. If upstream wants to depend on it, so be it. If they don't care about this problem at all, we'll need another solution.

[tornaria]

I agree that we should wait for comments for upstream, although meanwhile sagemath is broken in a very annoying way.

About your comment on the optional use of cysignals here's my take:

• pure python offers a way to install signal handlers via signal.signal(), and so for a pure python program that installs a custom sigint handler, saving and restoring python-level signals is necessary and good enough (no need to use sigaction). For example:

$ 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]: def h(x,y): print("Our custom handler received a sigint", x, y); raise KeyboardInterrupt

In [2]: import signal

In [3]: signal.signal(signal.SIGINT, h)
Out[3]: <cyfunction python_check_interrupt at 0x7f12316bcba0>

In [4]: while True: pass
^COur custom handler received a sigint 2 <frame at 0x55c760ab21c0, file '<ipython-input-4-b16dc615ea65>', line 1, code <module>>
---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
Input In [4], in <module>
----> 1 while True: pass

Input In [1], in h(x, y)
----> 1 def h(x,y): print("Our custom handler received a sigint", x, y); raise KeyboardInterrupt

KeyboardInterrupt: 

In [5]: 

The above snippet is broken with the version of prompt_toolkit that we ship in void, but it works with my patch (even without cysignals).

• when a python program uses sigaction to install a custom signal handler, then yes, saving and restoring with sigaction is the only way to fix this issue. Unfortunately python doesn't offer any standard way to do that. The only package I know uses sigaction to change the sigint handler is cysignals. But if cysignals is being used to change the sigint handler, then for sure cysignals is available to save and restore so we are good!

Furthermore, if managing signals set with sigaction is the right thing to do, it shouldn't be done only if the user chooses to install cysignals.

Thus, either cysignals should become a hard requirement of prompt_toolkit or it should have its own method to save and restore these signals.

I don't think either is necessary and presumably cysignals supports less architectures than prompt_toolkit (indeed sigaction only makes sense on posix); having its own method requires C code, that would be very simple to copy from cysignals.pysignals except prompt_toolkit doesn't compile any C code so why make building it more complicated for no good reason).

In case another program or python package installs a custom sigint handler and suffers of this issue, we can figure out how to fix it (for instance, we make that package depend on python3-cysignals). Right now there aren't many packages in void that use prompt_toolkit. In any case, if they break because of this they are already broken, my fix won't make them more broken but possibly less broken.

We really need some guidance from upstream about whether they intend to care about this problem at all and, if so, how they intend to solve it.

Sure.

I don't want to maintain indefinitely a custom prompt_toolkit patch that pulls in cysignals to manage these signals. If upstream wants to depend on it, so be it. If they don't care about this problem at all, we'll need another solution.

I can help to maintain the patch, at least for the time being while the issue is (hopefully) resolved upstream, and I can help to figure out another solution if this one doesn't work in the end.

[ahesford]

I understand the isssue and how your patch fixes it. Unless upstream merges your proposed handler changes, I will not accept a Void patch that does the same. The two options I will accept before upstream makes a decision are:

1. Your original patch to change to change the default SIGINT handling behavior from True to False, effectively restoring prior behavior and leaving any custom handlers untouched; or
2. A modification of your proposal that drops the conditional cysignals dependency and simply saves and restores Python signals in the prompt_toolkit event loop. (This will not fix your custom SIGINT problem in C extensions and probably is probably worse than Option 1 from your perspective.)

Option 1 may cause IPython to crash when it receives SIGINT, but that is functionally no worse than a revert to a prior version of prompt_toolkit and I can live with it.

Whether cysignals supports fewer platforms than prompt_toolkit is irrelevant to the discussion of whether it should be a hard dependency of prompt_toolkit. Python dependencies can be specified per-platform, so cysignals can always be restricted to the platforms where it can be built. However, I suspect that the need to maintain (and test) a platform-specific dependence on cysignals may make upstream less likely to accept this approach. For the same reason, I imagine upstream may be disinclined to incorporate its own compiled extension to manage sigaction.

The optional dependency on cysignals is unattractive because there are many different ways to set handlers via sigaction with Python: by compiling a C extension directly, by doing it directly in a Cython module that may have broader purposes or by using some FFI like the built-in ctypes. The population of people who rely on cysignals is likely to be small; having prompt_toolkit take special branches in every iteration of the event loop on the off chance that an arbitrary package is installed does not seem to be the right approach and certainly doesn't cover these alternatives.

Upstream needs to decide whether they even care about this problem and, if so, how to solve it. They might

• Declare cysignals as the one true way to manage sigaction and add a hard dependency on platforms that can use it
• Implement their own approach to save/restore as they see fit
• Find some other means to save/restore signals
• Accept your conditional approach, which solves the sigaction problem for some subset of people who may have actually defined a custom handler
  I'm not going to decide among these for upstream.

If you do PR your optional approach upstream, you should probably define a package option to declare the dependency so that the package can advertise the possibility.

[tornaria]

1. Your original patch to change to change the default SIGINT handling behavior from `True` to `False`, effectively restoring prior behavior and leaving any custom handlers untouched; or

Let's do this, I've force-pushed my original patch. This is completely acceptable for sagemath.

Option 1 may cause IPython to crash when it receives SIGINT, but that is functionally no worse than a revert to a prior version of prompt_toolkit and I can live with it.

I can live with this too. I hadn't even noticed this before I looked at the xonsh bug report.

[...]

having prompt_toolkit take special branches in every iteration of the event loop on the off chance that an arbitrary package is installed does not seem to be the right approach and certainly doesn't cover these alternatives.

Just for the record, assuming I understand the way python imports work, my patch doesn't add branches in the event loop. It tries to import cysignals.pysignals ONCE when prompt_toolkit is imported for the first time. If that fails, it will import instead alternative pure python functions (not using cysignals / not preserving the os-level sigint handler). These functions are unconditionally called once when prompt starts (to save the sigint handler) and once when the prompt finishes (to restore the sigint handler). The event loop runs all the prompt editing, completion, etc, etc, but this will not mess with signals at all.

If you do PR your optional approach upstream, you should probably define a package option to declare the dependency so that the package can advertise the possibility.

I'll wait for comments first before doing a PR. I might ask you for help then, since my understanding of python packaging and hard vs optional dependencies is almost nil.

[ahesford]

Thanks for entertaining my debate.

If you think that I can provide assistance, feel free to tag me in a PR or ping me on IRC.