Skip to content
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

Fix various invalid uses of sig_on() #27060

Closed
jdemeyer opened this issue Jan 15, 2019 · 14 comments
Closed

Fix various invalid uses of sig_on() #27060

jdemeyer opened this issue Jan 15, 2019 · 14 comments

Comments

@jdemeyer
Copy link

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.

Component: cython

Author: Jeroen Demeyer

Branch/Commit: 9fffd00

Reviewer: Timo Kaufmann

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

@jdemeyer jdemeyer added this to the sage-8.6 milestone Jan 15, 2019
@jdemeyer
Copy link
Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2019

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

ab966adFix various invalid uses of sig_on()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2019

Commit: ab966ad

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2019

Changed commit from ab966ad to 9fffd00

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2019

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

9fffd00Fix various invalid uses of sig_on()

@embray
Copy link
Contributor

embray commented Jan 15, 2019

comment:5

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.

@embray embray modified the milestones: sage-8.6, sage-8.7 Jan 15, 2019
@timokau
Copy link
Contributor

timokau commented Jan 17, 2019

comment:6

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...

@timokau
Copy link
Contributor

timokau commented Jan 17, 2019

Reviewer: Timo Kaufmann

@embray
Copy link
Contributor

embray commented Jan 18, 2019

comment:8

Could you say a little bit more about why these sig_on() are not correct? Is it because they're releasing the GIL inappropriately?

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

comment:10

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.

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Jan 18, 2019

comment:12

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

@vbraun
Copy link
Member

vbraun commented Jan 26, 2019

Changed branch from u/jdemeyer/fix_various_invalid_uses_of_sig_on__ to 9fffd00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants