Skip to content

Add general signal hook but keep pari #181

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Changelog
^^^^^^^^^^^^^^^^^^^^^^^^^

* Replace `fprintf` by calls to `write`, which is async-signal-safe according to POSIX. [#162]
* Introduce a general hook to interface with custom signal handling


1.11.2 (2021-12-15)
Expand Down
55 changes: 55 additions & 0 deletions docs/source/interrupt.rst
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,58 @@ can conditionally call ``sig_on()`` and ``sig_off()``::

This should only be needed if both the check (``n > 100`` in the example) and
the code inside the ``sig_on()`` block take very little time.

.. _section_add_custom_signals:

Using custom blocking and signal handlers
-----------------------------------------

The following illustrates how signals can be held back similar to
``sig_block`` and ``sig_unblock``. The number theory libary PARI/GP
defines a variable, which indicates that the execution should not
currently be interrupted. Another variable is used to indicate a pending signal,
so that PARI/GP can treat it.

Other external libraries might use a similar scheme.
Here we indicate this might work::

from cysignals.signals cimport sig_on, sig_off, add_custom_signals

cdef extern from "stdio.h":
void sleep(int)

cdef int SIGINT_block = 0
cdef int SIGINT_pending = 0

cdef int signal_is_blocked():
return SIGINT_block

cdef void signal_unblock():
global SIGINT_block
SIGINT_block = 0

cdef void set_pending_signal(int sig):
global SIGINT_pending
SIGINT_pending = sig

# Use the hook provided by cysignals.
add_custom_signals(&signal_is_blocked, &signal_unblock, &set_pending_signal)

def foo(size_t b, int blocked):
global SIGINT_block, SIGINT_pending
sig_on()
SIGINT_block = blocked
for i in range(b):
sleep(1)
if SIGINT_pending:
SIGINT_block = 0
SIGINT_pending = 0
raise KeyboardInterrupt("interrupt was held back")
SIGINT_block = 0
sig_off()
return

In the above scenario ``foo(10, 0)`` would just wait for 10 seconds,
while allowing interrupts. ``foo(10, 1)`` blocks the interrupt
until the end of the second. The pending signal is then treated
with a custom message.
37 changes: 36 additions & 1 deletion src/cysignals/implementation.c
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,37 @@ static int PARI_SIGINT_block = 0;
static int PARI_SIGINT_pending = 0;
#define paricfg_version NULL
#endif

// Custom signal handling of other packages.
#define MAX_N_CUSTOM_HANDLERS 16

static int (*custom_signal_is_blocked_pts[MAX_N_CUSTOM_HANDLERS])();
static void (*custom_signal_unblock_pts[MAX_N_CUSTOM_HANDLERS])();
static void (*custom_set_pending_signal_pts[MAX_N_CUSTOM_HANDLERS])(int);
static int n_custom_handlers = 0;

int custom_signal_is_blocked(){
// Check if a custom block is set.
for(int i = 0; i < n_custom_handlers; i++){
if (custom_signal_is_blocked_pts[i]())
return 1;
}
return 0;
}

void custom_signal_unblock(){
// Unset all custom blocks.
for(int i = 0; i < n_custom_handlers; i++)
custom_signal_unblock_pts[i]();
}


void custom_set_pending_signal(int sig){
// Set a pending signal to custom handlers.
for(int i = 0; i < n_custom_handlers; i++)
custom_set_pending_signal_pts[i](sig);
}

#if HAVE_WINDOWS_H
/* We must include <windows.h> after <pari.h>
* See https://github.com/sagemath/cysignals/issues/107 */
Expand Down Expand Up @@ -214,7 +245,7 @@ static void cysigs_interrupt_handler(int sig)

if (cysigs.sig_on_count > 0)
{
if (!cysigs.block_sigint && !PARI_SIGINT_block)
if (!cysigs.block_sigint && !PARI_SIGINT_block && !custom_signal_is_blocked())
{
/* Raise an exception so Python can see it */
do_raise_exception(sig);
Expand All @@ -238,6 +269,7 @@ static void cysigs_interrupt_handler(int sig)
{
cysigs.interrupt_received = sig;
PARI_SIGINT_pending = sig;
custom_set_pending_signal(sig);
}
}

Expand Down Expand Up @@ -400,6 +432,7 @@ static void _sig_on_interrupt_received(void)
cysigs.sig_on_count = 0;
cysigs.interrupt_received = 0;
PARI_SIGINT_pending = 0;
custom_signal_unblock();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a typo in this line: it should have been

    custom_set_pending_signal(0);

instead.

In fact, I was having some issues with cysignals 1.12.2 (some doctest failures having to do with alarms, etc) and changing this seems to sovle them.

@kliem can you confirm this is correct?

@antonio-rojas I think this is the fix we were missing. I'm testing, will do a PR tomorrow. Because of the PARI_SIGINT_pending = 0; line, the typo doesn't cause trouble (i.e., until that line is removed).

To be clear, this is the patch I'm testing on top of 1.12.2:

--- a/src/cysignals/implementation.c
+++ b/src/cysignals/implementation.c
@@ -591,7 +591,7 @@ static void _sig_on_interrupt_received(void)
     do_raise_exception(cysigs.interrupt_received);
     cysigs.sig_on_count = 0;
     cysigs.interrupt_received = 0;
-    custom_signal_unblock();
+    custom_set_pending_signal(0);
 
 #if HAVE_SIGPROCMASK
     sigprocmask(SIG_SETMASK, &oldset, NULL);

Choose a reason for hiding this comment

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

Doesn't quite work for me. Now instead of crashing, tests hang and timeout after 30 minutes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything works "fine" for me (still getting occasional random failures, but that's not new).

The only difference between cysignals 1.11.4 and 1.12.2 in this respect is this change. Although with 1.11.4, in effect, it is both custom_set_pending_signal(0); and custom_signal_unblock(); but I don't think the unblock there is correct.

Are you using sagemath/cypari2#167 (included in cypari2 2.2.1)? This could make a difference, maybe. I'm not sure if this code is compiled with legacy_implicit_noexcept, you could check the generated .c file to see if it's "clean" as it should be.

The other patch I'm using on cysignals (already for 1.11.4) is: https://github.com/void-linux/void-packages/blob/master/srcpkgs/python3-cysignals/patches/verify_exc_value.patch

Choose a reason for hiding this comment

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

Are you using sagemath/cypari2#167 (included in cypari2 2.2.1)? This could make a difference, maybe. I'm not sure if this code is compiled with legacy_implicit_noexcept, you could check the generated .c file to see if it's "clean" as it should be.

I'm getting the same with cypari2 2.2.0 (+pari 2.17 patches)

Copy link
Member

@dimpase dimpase Jan 7, 2025

Choose a reason for hiding this comment

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

I've released cypari2 2.2.1 - which supports pari 2.17

Choose a reason for hiding this comment

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

ok, not sure what happened before (possibly some memory issues), but it seems to be working now, except that the random failure I reported in sagemath/sage#38749 (comment) is now permanent (regardless of the used seed)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, not sure what happened before (possibly some memory issues), but it seems to be working now, except that the random failure I reported in sagemath/sage#38749 (comment) is now permanent (regardless of the used seed)

Great! I have a PR almost ready to submit (including a test to trigger this bug with cypari2).

As for the remaining issue, I'm getting the same, and I'm working on it. This appears to be related to #215. The implementation of sig_occurred() seems to be incorrect, in the sense that sig_occurred() will sometimes return an old exception that has been already handled.

Copy link
Member

Choose a reason for hiding this comment

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

is it a basically a 1-line PR (+ the test)?

Copy link
Contributor

Choose a reason for hiding this comment

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

is it a basically a 1-line PR (+ the test)?

Yes, I wanted to test it further. I'm also working on the remaining issue which seems more tricky, but I think I have a reasonable fix. Hopefully this might get rid of all the stupid heisenbugs that have been pestering us.

I was actually trying to push this to GH a moment ago, but it seems there is a major outage right now (git pull/push from/to github, are not working atm).


#if HAVE_SIGPROCMASK
sigprocmask(SIG_SETMASK, &oldset, NULL);
Expand All @@ -412,9 +445,11 @@ static void _sig_on_recover(void)
{
cysigs.block_sigint = 0;
PARI_SIGINT_block = 0;
custom_signal_unblock();
cysigs.sig_on_count = 0;
cysigs.interrupt_received = 0;
PARI_SIGINT_pending = 0;
custom_set_pending_signal(0);

#if HAVE_SIGPROCMASK
/* Reset signal mask */
Expand Down
4 changes: 4 additions & 0 deletions src/cysignals/signals.pxd.in
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ cdef extern from "macros.h" nogil:
int sig_str_no_except "sig_str"(const char*)
int sig_check_no_except "sig_check"()

# This function adds custom block/unblock/pending.
cdef int add_custom_signals(int (*custom_signal_is_blocked)(),
void (*custom_signal_unblock)(),
void (*custom_set_pending_signal)(int)) except -1

# This function does nothing, but it is declared cdef except *, so it
# can be used to make Cython check whether there is a pending exception
Expand Down
31 changes: 31 additions & 0 deletions src/cysignals/signals.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ cdef extern from "implementation.c":
# PARI version string; NULL if compiled without PARI support
const char* paricfg_version

int (**custom_signal_is_blocked_pts)()
void (**custom_signal_unblock_pts)()
void (**custom_set_pending_signal_pts)(int)
int n_custom_handlers
int MAX_N_CUSTOM_HANDLERS


def _pari_version():
"""
Expand All @@ -74,6 +80,31 @@ def _pari_version():
return v.decode('ascii')


cdef int add_custom_signals(int (*custom_signal_is_blocked)(),
void (*custom_signal_unblock)(),
void (*custom_set_pending_signal)(int)) except -1:
"""
Add an external block/unblock/pending to cysignals.

INPUT:

- ``custom_signal_is_blocked`` -- returns whether signals are currently blocked.

- ``custom_signal_unblock`` -- unblocks signals

- ``custom_set_pending_signal`` -- set a pending signal in case of blocking
"""
global n_custom_handlers
if n_custom_handlers == MAX_N_CUSTOM_HANDLERS:
raise IndexError("maximal number of custom handlers exceeded")

custom_signal_is_blocked_pts[n_custom_handlers] = custom_signal_is_blocked
custom_signal_unblock_pts[n_custom_handlers] = custom_signal_unblock
custom_set_pending_signal_pts[n_custom_handlers] = custom_set_pending_signal

n_custom_handlers += 1


class AlarmInterrupt(KeyboardInterrupt):
"""
Exception class for :func:`alarm` timeouts.
Expand Down