sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Invalid use of sig_on() in acb_calc_func_callback #27428

Open jdemeyer opened 5 years ago

jdemeyer commented 5 years ago

The function acb_calc_func_callback ends with a sig_on() statement. This makes absolutely no sense at all (see https://cysignals.readthedocs.io/en/latest/interrupt.html#using-sig-on-and-sig-off). This was introduced in #24686.

In that same function, there is also a bare except: which should be fixed to except BaseException: (you really want to catch all exceptions, so it's better to be explicit about that).

CC: @mezzarobba

Component: cython

Issue created by migration from https://trac.sagemath.org/ticket/27428

jdemeyer commented 5 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 The function `acb_calc_func_callback` ends with a `sig_on()` statement. This makes absolutely no sense at all. This was introduced in #24686.
+
+In that same function, there is also a bare `except:` which should be fixed.
jdemeyer commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-The function `acb_calc_func_callback` ends with a `sig_on()` statement. This makes absolutely no sense at all. This was introduced in #24686.
+The function `acb_calc_func_callback` ends with a `sig_on()` statement. This makes absolutely no sense at all (see https://cysignals.readthedocs.io/en/latest/interrupt.html#using-sig-on-and-sig-off). This was introduced in #24686.

 In that same function, there is also a bare `except:` which should be fixed.
mezzarobba commented 5 years ago

Replying to @jdemeyer:

The function acb_calc_func_callback ends with a sig_on() statement. This makes absolutely no sense at all (see https://cysignals.readthedocs.io/en/latest/interrupt.html#using-sig-on-and-sig-off).

Thanks for the notice. I think I'm unable to fix the issue by myself, unfortunately.

Is there a description of sig_on()/sig_off() that explains what they do and where the rules listed in the cysignal manual come from, instead of just saying what one should and shouldn't do? I don't remember the rule that “sig_off() should be called before the function that called sig_on() returns” being there when we wrote that code (but I may well have missed it).

What is the proper way to handle the case of (i) an external C library (ii) that performs callbacks into Python code (iii) but not very frequent ones, so that one would like the code running between two callbacks to be interruptible?

jdemeyer commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 The function `acb_calc_func_callback` ends with a `sig_on()` statement. This makes absolutely no sense at all (see https://cysignals.readthedocs.io/en/latest/interrupt.html#using-sig-on-and-sig-off). This was introduced in #24686.

-In that same function, there is also a bare `except:` which should be fixed.
+In that same function, there is also a bare `except:` which should be fixed to `except BaseException:` (you really want to catch all exceptions, so it's better to be explicit about that).
jdemeyer commented 5 years ago
comment:4

Replying to @mezzarobba:

Is there a description of sig_on()/sig_off() that explains what they do and where the rules listed in the cysignal manual come from

Not really, but the important bit to know is that it relies on setjmp()/longjmp(). So the restriction comes from setjmp(). From the setjmp man page:

If the function which called setjmp() returns before longjmp() is called, the behavior is undefined.

I don't remember the rule that “sig_off() should be called before the function that called sig_on() returns” being there when we wrote that code

How old is your code? That piece of documentation dates from #10109 :-)

What is the proper way to handle the case of (i) an external C library (ii) that performs callbacks into Python code (iii) but not very frequent ones, so that one would like the code running between two callbacks to be interruptible?

It's complicated. The hardest part is dealing with exceptions. I see that you already have provisions for that using the ctx object. An alternative way for dealing with exceptions is sig_error(). Note that you still have a lot of Python code unguarded by try/except which could raise exceptions (even an assert statement!).

The easy part is making sure that interrupts cannot happen while executing Python code. That can be done with sig_block()/sig_unblock().

a396f486-5fc5-40c2-a542-2e12efb37784 commented 5 years ago

Branch: u/gh-Hrishabh-yadav/filter_kruskal_spanning_tree

jdemeyer commented 5 years ago
comment:6

Wrong ticket?

jdemeyer commented 5 years ago

Changed branch from u/gh-Hrishabh-yadav/filter_kruskal_spanning_tree to none

a396f486-5fc5-40c2-a542-2e12efb37784 commented 5 years ago
comment:7

Replying to @jdemeyer:

Wrong ticket?

I know, It was a mistake.. How do I remove a branch from a ticket

a396f486-5fc5-40c2-a542-2e12efb37784 commented 5 years ago
comment:8

Replying to @Hrishabh-yadav:

Replying to @jdemeyer:

Wrong ticket?

I know, It was a mistake.. How do I remove a branch from a ticket

Thanks

embray commented 5 years ago
comment:9

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

embray commented 5 years ago
comment:10

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).