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

alarm() doesn't work for Cython code #13311

Closed
sagetrac-alexc mannequin opened this issue Jul 31, 2012 · 29 comments
Closed

alarm() doesn't work for Cython code #13311

sagetrac-alexc mannequin opened this issue Jul 31, 2012 · 29 comments

Comments

@sagetrac-alexc
Copy link
Mannequin

sagetrac-alexc mannequin commented Jul 31, 2012

The following code does not work as intended (I imagine), i.e. it does not interrupt the computation after 3 seconds.

#!/usr/bin/python -tt

from sage.all import *
import time

def main():
  alarm(3)
  try:
    EllipticCurve([123456, 789011]).rank()
    #time.sleep(5) #code works as intended if the above command is replaced with this
  except KeyboardInterrupt:
    print 'Took too long'

# This is the standard boilerplate that calls the main() function.
if __name__ == '__main__':
  main()

The problem is that the c_lib interrupt handling code doesn't treat the SIGALRM signal, this is fixed in the attached patch. It also changes some exception classes (instead of raising RuntimeError for every signal). Another cool new feature is that alarm() now works with a floating-point number of seconds.

Apply attachment: 13311_sigalrm.patch

Depends on #14029

CC: @pjbruin

Component: c_lib

Keywords: alarm signal interrupt

Author: Jeroen Demeyer

Reviewer: Peter Bruin

Merged: sage-5.13.beta3

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

@sagetrac-alexc sagetrac-alexc mannequin added this to the sage-5.11 milestone Jul 31, 2012
@JohnCremona
Copy link
Member

comment:1

I don't know anything about alarm(). In this case mwrank is being used to compute the rank and it takes more then 3s.

@sagetrac-alexc
Copy link
Mannequin Author

sagetrac-alexc mannequin commented Jul 31, 2012

comment:2

Replying to @JohnCremona:

I don't know anything about alarm(). In this case mwrank is being used to compute the rank and it takes more then 3s.

alarm(timelimit) is supposed to raise a KeyboardInterrupt exception after timelimit seconds. For some reason, despite mwrank running for more than 3 seconds, the computation isn't interrupted. Documentation for alarm can be found here: http://www.sagemath.org/doc/reference/sage/misc/misc.html#sage.misc.misc.alarm

@koffie
Copy link
Contributor

koffie commented Jul 31, 2012

comment:3

The following might help to locate the problem: http://www.sagemath.org/doc/developer/coding_in_cython.html#using-sig-on-and-sig-off

@jdemeyer
Copy link
Contributor

comment:4

The problem is that the SIGALRM handling is completely independent of the usual signal handling, hence it doesn't work. It simply raises a Python-level KeyboardInterrupt, but that's it. It doesn't interact with any of the sophisticated sig_on()/sig_off() stuff.

As an ugly work-around, try using signal_after_delay() from sage.tests.interrupt, which generates an actual interrupt.

@jdemeyer jdemeyer changed the title alarm doesn't work with some elliptic curve functions alarm() doesn't work for Cython code Sep 27, 2012
@jdemeyer
Copy link
Contributor

Changed keywords from alarm elliptic curve curves ellipticcurve to alarm signal interrupt

@jdemeyer
Copy link
Contributor

comment:5

This has absolutely nothing to do with elliptic curves...

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 12, 2013

comment:6

Replying to @jdemeyer:

The problem is that the SIGALRM handling is completely independent of the usual signal handling, hence it doesn't work. [...]

As an ugly work-around, try using signal_after_delay() from sage.tests.interrupt, which generates an actual interrupt.

A "cleaner" work-around is to use the @fork decorator, such that the computation (implemented in Cython or e.g. C/C++) runs in a subprocess, while alarm() is used in the main (Python) process; cf. this solution on ask.sagemath.

@jdemeyer
Copy link
Contributor

comment:7

Replying to @nexttime:

A "cleaner" work-around is to use the @fork decorator, such that the computation (implemented in Cython or e.g. C/C++) runs in a subprocess, while alarm() is used in the main (Python) process; cf. this solution on ask.sagemath.

I disagree with "cleaner" but that would work, yes.

@jdemeyer
Copy link
Contributor

Dependencies: #14029

@jdemeyer
Copy link
Contributor

Author: Jeroen Demeyer

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 13, 2013

comment:10

Minor things:

"The latter signal also redirect stdin from /dev/null, to cause interactive sessions to exit also."

-> "The latter signal also redirects stdin from /dev/null, to cause interactive sessions to exit."

s/time out/time-out/ (or timeout, which is also used later on) a couple of times, and in the doctest error message s/Time out/Timed out/ (consistent with e.g. Interrupted).

In sig_raise_exception(), shouldn't the message in raise SystemError("unknown signal") contain the signal number?

@jdemeyer

This comment has been minimized.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 15, 2013

Replying to @jdemeyer:

Another cool new feature is that alarm() now works with a floating-point number of seconds.

Maybe on a real-time OS ... ;-)

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 15, 2013

comment:13

P.S.: Patch looks ok, but I haven't had the time to test this (and #14029) yet.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 15, 2013

comment:14

Replying to @nexttime:

In sig_raise_exception(), shouldn't the message in raise SystemError("unknown signal") contain the signal number?

Ok, fixed now. Or more nit-picking:

Maybe

    raise SystemError("Received unknown signal number %s"%sig)

?

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@pjbruin
Copy link
Contributor

pjbruin commented Oct 7, 2013

Work Issues: rebase

@pjbruin
Copy link
Contributor

pjbruin commented Oct 7, 2013

comment:17

This doesn't apply anymore to 5.12.rc1 + #14029; the patch fails on sage/ext/c_lib.pyx and sage/structure/parent.pyx.

@jdemeyer
Copy link
Contributor

Attachment: 13311_sigalrm.patch.gz

@pjbruin
Copy link
Contributor

pjbruin commented Oct 29, 2013

comment:19

Generating a SIGALRM outside sig_on()...sig_off() calls PyErr_SetInterrupt(), which causes Python to raise a KeyboardInterrupt:

sage: alarm(1) 
sage: # wait 1 second
KeyboardInterrupt

The same thing already happens without this patch. Wouldn't it make sense to also raise an AlarmInterrupt outside sig_on()...sig_off(), too?

@jdemeyer
Copy link
Contributor

comment:20

Replying to @pjbruin:

Generating a SIGALRM outside sig_on()...sig_off() calls PyErr_SetInterrupt(), which causes Python to raise a KeyboardInterrupt

No, it doesn't. It causes IPython to print the string "KeyboardInterrupt". See the difference with

sage: alarm(0.5); sleep(1)
---------------------------------------------------------------------------
AlarmInterrupt                            Traceback (most recent call last)
<ipython-input-3-854e6c430970> in <module>()
----> 1 alarm(RealNumber('0.5')); sleep(Integer(1))

/scratch/release/merger/sage-5.13.beta1/local/lib/python2.7/site-packages/sage/ext/c_lib.so in sage.ext.c_lib.sage_python_check_interrupt (sage/ext/c_lib.c:1634)()

/scratch/release/merger/sage-5.13.beta1/local/lib/python2.7/site-packages/sage/ext/c_lib.so in sage.ext.c_lib.sig_raise_exception (sage/ext/c_lib.c:894)()

AlarmInterrupt: 

@pjbruin
Copy link
Contributor

pjbruin commented Oct 29, 2013

comment:21

Replying to @jdemeyer:

Replying to @pjbruin:

Generating a SIGALRM outside sig_on()...sig_off() calls PyErr_SetInterrupt(), which causes Python to raise a KeyboardInterrupt

No, it doesn't. It causes IPython to print the string "KeyboardInterrupt".

I was mislead by the Python documentation, which says that PyErr_SetInterrupt() causes a KeyboardInterrupt to be raised the next time PyErr_CheckSignals() is called. In fact this only describes what Python's default SIGINT handler does, which we don't use.

OK, so IPython prints the string "KeyboardInterrupt" in an except KeyboardInterrupt block when it is reading input. Do I understand correctly that the KeyboardInterrupt is in this case actually an AlarmInterrupt and triggers this except block because it inherits from KeyboardInterrupt?

@jdemeyer
Copy link
Contributor

comment:22

Replying to @pjbruin:

I was mislead by the Python documentation, which says that PyErr_SetInterrupt() causes a KeyboardInterrupt to be raised the next time PyErr_CheckSignals() is called. In fact this only describes what Python's default SIGINT handler does, which we don't use.

Indeed. The Python SIGINT handler is changed in _init_csage() from devel/sage/sage/ext/c_lib.pyx.

OK, so IPython prints the string "KeyboardInterrupt" in an except KeyboardInterrupt block when it is reading input. Do I understand correctly that the KeyboardInterrupt is in this case actually an AlarmInterrupt and triggers this except block because it inherits from KeyboardInterrupt?

Exactly.

@pjbruin
Copy link
Contributor

pjbruin commented Oct 30, 2013

comment:23

Replying to @jdemeyer:

No, it doesn't. It causes IPython to print the string "KeyboardInterrupt".

Given that an interrupt is not necessarily generated from the keyboard, it would be more logical from the user's perspective if IPython would print either (1) a generic message like "Interrupt", (2) a different string for KeyboardInterrupt and AlarmInterrupt, or even (3) nothing (Bash just prints a newline when you press ^C and nothing when you press ^\.)

I would be in favour of (2); to implement this, IPython could print E.__class__.__name__ inside except KeyboardInterrupt as E instead of the constant string "KeyboardInterrupt".

In principle it would also make more sense if KeyboardInterrupt and AlarmInterrupt would derive from a common Interrupt exception, but this is impossible since KeyboardInterrupt is a built-in Python exception.

@jdemeyer
Copy link
Contributor

comment:24

Perhaps, but I don't believe it's worth the trouble to patch IPython for this.

@pjbruin
Copy link
Contributor

pjbruin commented Oct 30, 2013

comment:25

Replying to @jdemeyer:

Perhaps, but I don't believe it's worth the trouble to patch IPython for this.

Maybe, maybe not; in any case we can always do it later.

That issue aside, the patch looks good, behaves as expected and passes doctests.

@pjbruin
Copy link
Contributor

pjbruin commented Oct 30, 2013

Reviewer: Peter Bruin

@jdemeyer
Copy link
Contributor

Changed work issues from rebase to none

@jdemeyer
Copy link
Contributor

jdemeyer commented Nov 6, 2013

Merged: sage-5.13.beta3

@jdemeyer
Copy link
Contributor

jdemeyer commented Dec 1, 2014

comment:28

I finally sent a patch to IPython about this: ipython/ipython#7069

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