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:

Status badges

Description (last modified by jdemeyer)

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 jdemeyer

  • Branch set to u/jdemeyer/fix_various_invalid_uses_of_sig_on__

comment:2 Changed 3 years ago by git

  • Commit set to ab966ad65904d7858c4adc14c9d83e0819953967

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​ab966adFix various invalid uses of sig_on()

comment:3 Changed 3 years ago by git

  • Commit changed from ab966ad65904d7858c4adc14c9d83e0819953967 to 9fffd00d2f52ed0c2b0d584870c15a22771eebb9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​9fffd00Fix various invalid uses of sig_on()

comment:4 Changed 3 years ago by jdemeyer

  • Status changed from new to needs_review

comment:5 Changed 3 years ago by embray

  • 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 gh-timokau

  • 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 gh-timokau

  • Reviewers set to Timo Kaufmann

comment:8 follow-up: Changed 3 years ago by 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?

comment:9 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:10 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

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 jdemeyer

  • Description modified (diff)

comment:12 Changed 3 years ago by embray

I see what you're saying now--thank you.

comment:13 Changed 3 years ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.