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: | sage-8.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 non-trivial 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 sage-8.6 to sage-8.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 sage-pending or sage-wishlist.
comment:6 Changed 3 years ago by
- Status changed from needs_review to positive_review
On the macro-scale 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 follow-up: ↓ 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 now--thank 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()