Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#13311 closed defect (fixed)

alarm() doesn't work for Cython code

Reported by: alexc Owned by: jdemeyer
Priority: major Milestone: sage-5.13
Component: c_lib Keywords: alarm signal interrupt
Cc: pbruin Merged in: sage-5.13.beta3
Authors: Jeroen Demeyer Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14029 Stopgaps:

Description (last modified by jdemeyer)

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 13311_sigalrm.patch

Attachments (1)

13311_sigalrm.patch (31.5 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 follow-up: Changed 8 years ago by cremona

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

comment:2 in reply to: ↑ 1 Changed 8 years ago by alexc

Replying to cremona:

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

comment:3 Changed 8 years ago by mderickx

comment:4 follow-up: Changed 7 years ago by jdemeyer

  • Summary changed from alarm doesn't work with some elliptic curve functions to alarm() doesn't work for Cython code

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.

comment:5 Changed 7 years ago by jdemeyer

  • Component changed from elliptic curves to c_lib
  • Keywords signal interrupt added; elliptic curve curves ellipticcurve removed
  • Owner changed from cremona to jdemeyer

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

comment:6 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by leif

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.

comment:7 in reply to: ↑ 6 Changed 7 years ago by jdemeyer

Replying to leif:

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.

comment:8 Changed 7 years ago by jdemeyer

  • Dependencies set to #14029

comment:9 Changed 7 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

comment:10 follow-up: Changed 7 years ago by leif

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?

comment:11 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:12 in reply to: ↑ description Changed 7 years ago by leif

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

Last edited 7 years ago by leif (previous) (diff)

comment:13 Changed 7 years ago by leif

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

comment:14 in reply to: ↑ 10 Changed 7 years ago by leif

Replying to leif:

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)

?  

comment:15 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:16 Changed 6 years ago by pbruin

  • Cc pbruin added

comment:17 Changed 6 years ago by pbruin

  • Status changed from needs_review to needs_work
  • Work issues set to rebase

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

Changed 6 years ago by jdemeyer

comment:18 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:19 follow-up: Changed 6 years ago by pbruin

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?

comment:20 in reply to: ↑ 19 ; follow-ups: Changed 6 years ago by jdemeyer

Replying to pbruin:

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: 

comment:21 in reply to: ↑ 20 ; follow-up: Changed 6 years ago by pbruin

Replying to jdemeyer:

Replying to pbruin:

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?

comment:22 in reply to: ↑ 21 Changed 6 years ago by jdemeyer

Replying to pbruin:

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.

comment:23 in reply to: ↑ 20 Changed 6 years ago by pbruin

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.

comment:24 follow-up: Changed 6 years ago by jdemeyer

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

comment:25 in reply to: ↑ 24 Changed 6 years ago by pbruin

  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

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.

comment:26 Changed 6 years ago by jdemeyer

  • Work issues rebase deleted

comment:27 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.13.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:28 Changed 5 years ago by jdemeyer

I finally sent a patch to IPython about this: https://github.com/ipython/ipython/pull/7069

Note: See TracTickets for help on using tickets.