Opened 3 years ago
Closed 3 years ago
#27060 closed defect (fixed)
Fix various invalid uses of sig_on()
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  cython  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Timo Kaufmann 
Report Upstream:  N/A  Work issues:  
Branch:  9fffd00 (Commits, GitHub, GitLab)  Commit:  9fffd00d2f52ed0c2b0d584870c15a22771eebb9 
Dependencies:  Stopgaps: 
Description (last modified by )
In order to debug crashes with cypari2 (#26442), I added some checks to cypari2. But these checks are now failing because sig_on()
is regularly used badly inside Sage.
As documented in the cysignals documentation, code inside a sig_on()
/sig_off()
block should not manipulate Python objects. The reason is that an interrupt at that time could put Python objects in an invalid state. It is especially problematic if a garbage collection happens inside a sig_on()
block (because that takes a nontrivial amount of time and has a high probability of putting things in an invalid state).
This ticket fixes a few of those cases where garbage collection might happen.
Change History (13)
comment:1 Changed 3 years ago by
 Branch set to u/jdemeyer/fix_various_invalid_uses_of_sig_on__
comment:2 Changed 3 years ago by
 Commit set to ab966ad65904d7858c4adc14c9d83e0819953967
comment:3 Changed 3 years ago by
 Commit changed from ab966ad65904d7858c4adc14c9d83e0819953967 to 9fffd00d2f52ed0c2b0d584870c15a22771eebb9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9fffd00  Fix various invalid uses of sig_on()

comment:4 Changed 3 years ago by
 Status changed from new to needs_review
comment:5 Changed 3 years ago by
 Milestone changed from sage8.6 to sage8.7
Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sagepending or sagewishlist.
comment:6 Changed 3 years ago by
 Status changed from needs_review to positive_review
On the macroscale this LGTM. On the details, the person I'd choose to review this is you. But since you already made the commit...
comment:7 Changed 3 years ago by
 Reviewers set to Timo Kaufmann
comment:8 followup: ↓ 10 Changed 3 years ago by
Could you say a little bit more about why these sig_on()
are not correct? Is it because they're releasing the GIL inappropriately?
comment:9 Changed 3 years ago by
 Description modified (diff)
comment:10 in reply to: ↑ 8 Changed 3 years ago by
Replying to embray:
Could you say a little bit more about why these
sig_on()
are not correct? Is it because they're releasing the GIL inappropriately?
I updated the ticket description, I hope it is clearer now. It has nothing to do with the GIL.
comment:11 Changed 3 years ago by
 Description modified (diff)
comment:12 Changed 3 years ago by
I see what you're saying nowthank you.
comment:13 Changed 3 years ago by
 Branch changed from u/jdemeyer/fix_various_invalid_uses_of_sig_on__ to 9fffd00d2f52ed0c2b0d584870c15a22771eebb9
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Fix various invalid uses of sig_on()