classification
Title: Condition.wait() doesn't raise KeyboardInterrupt
Type: behavior Stage: committed/rejected
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Yuri.Bochkarev, gregory.p.smith, haypo, hobb0001, jyasskin, pitrou, rnk
Priority: normal Keywords: patch

Created on 2010-05-28 16:48 by hobb0001, last changed 2013-11-07 12:58 by Yuri.Bochkarev. This issue is now closed.

Files
File name Uploaded Description Edit
lock-interrupt.diff rnk, 2010-07-10 15:47 patch to make Python-level lock acquires interruptible review
lock-interrupt.diff rnk, 2010-07-10 17:28 Updated patch for RLock and docs review
lock-interrupt.diff rnk, 2010-07-10 20:32 retry lock acquisitions in RLock review
lock-interrupt-v4.diff rnk, 2010-08-15 04:51 review
Messages (22)
msg106678 - (view) Author: Mike Hobbs (hobb0001) Date: 2010-05-28 16:48
Condition.wait() without a timeout will never raise a KeyboardInterrupt:

cond = threading.Condition()
cond.acquire()
cond.wait()

*** Pressing Ctrl-C now does nothing ***



If you pass a timeout to Condition.wait(), however, it does behave as expected:

cond.wait()

^CTraceback (most recent call last):
  File "/usr/lib/python3.1/threading.py", line 242, in wait
    _sleep(delay)
KeyboardInterrupt



This may affect other problems reported with multiprocessing pools. Most notably:
http://bugs.python.org/issue8296
http://stackoverflow.com/questions/1408356
msg106680 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-28 17:40
In 3.2, even using a timeout doesn't make the call interruptible.
The solution would be to fix the internal locking APIs so that they handle incoming signals properly, and are able to return an error status.
msg106685 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-28 19:16
3.2 is interesting in that it introduces a new internal API: PyThread_acquire_lock_timed(). We can therefore change that API again before release without risking any compatibility breakage. Reid, would you want to work on this?
msg106705 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-05-29 02:59
I'd like to fix it, but I don't know if I'll be able to in time.  It was something that bugged me while running the threading tests while working on Unladen.

I'm imagining (for POSIX platforms) adding some kind of check for signals when the system call returns EINTR.  If the signal handler raises an exception, like an interrupt should raise a KeyboardInterrupt, we can just give a different return code and propagate the exception.

It also seems like this behavior can be extended gradually to different platforms, since I don't have the resources to change and test every threading implementation.
msg106715 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-29 11:44
> I'm imagining (for POSIX platforms) adding some kind of check for
> signals when the system call returns EINTR.  If the signal handler
> raises an exception, like an interrupt should raise a
> KeyboardInterrupt, we can just give a different return code and
> propagate the exception.

Yes, this is what I'm proposing too.

> It also seems like this behavior can be extended gradually to
> different platforms, since I don't have the resources to change and
> test every threading implementation.

There is only one active POSIX threading implementation in 3.2, in
Python/thread_pthread.h.
(and the only non-POSIX one is for Windows)
msg109883 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-07-10 15:47
Here's a patch that makes Python-level lock acquisitions interruptible for py3k.  There are many users of the C-level lock API, most of whom are not set up to deal with lock acquisition failure.  I decided to make a new API function and leave the others alone.

If possible, I think this should go out with 3.2.

In that case, I was wondering if I should merge PyThread_acquire_lock_timed with my new PyThread_acquire_lock_timed_intr, since PyThread_acquire_lock_timed wasn't available in 3.1.  Although it did go out in 2.7, we don't promise C API compatibility with the 2.x series, so I don't think it matters.

I've tested this patch on Mac OS X and Linux.  The whole test suite passes on both, along with the test that I added to test_threadsignals.py.

I added a noop compatibility wrapper to thread_nt.h, but I haven't tested it or built it.  When I get around to testing/fixing the subprocess patch on Windows, I'll make sure this works and the test is skipped.
msg109884 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-10 16:13
Oh, nice!

> I was wondering if I should merge PyThread_acquire_lock_timed with my
> new PyThread_acquire_lock_timed_intr, since PyThread_acquire_lock_timed 
> wasn't available in 3.1.

Yes, I think you should.

I haven't tried the patch, but it seems you got the logic right. There's a problem sometimes that you're using 2 spaces for indent rather than 4. Also, you forgot to update the RLock implementation in _threadmodule.c (and perhaps add another test for it).

(there are other modules which use the PyThread_acquire_lock API, but most of the time the locks are held for a short time (and shouldn't deadlock), which makes converting them less of a priority)
msg109898 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-07-10 17:28
Here is a new version of a patch that updates recursive locks to have the same behavior.  The pure Python RLock implementaiton should be interruptible by virtue of the base lock acquire primitive being interruptible.

I've also updated the relevant documentation I could find.  I've surely missed some, though.

I also got rid of the _intr version of lock acquisition and simply added a new parameter to the _timed variant.  The only two callers of it are the ones I updated in the _thread module.
msg109915 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-10 19:43
The RLock version gets different semantics in your patch: it doesn't retry acquiring when a signal is caught. Is there a reason for that?

By the way, your patch works fine under Windows.
msg109932 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-07-10 20:32
Oops, copy/paste oversight.  =/  I wrote a test to verify that it handles signals, and then retries the lock acquire.
msg109935 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-07-10 20:57
Also, thanks for the quick reviews!
msg110007 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-11 13:36
The latest patch looks good to me.
msg110105 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-12 16:08
Actually, there is a problem in Lock.acquire and RLock.acquire. If a signal occurs and signal handling returns successfully, acquiring the lock will be retried without decrementing the timeout first. Therefore, we may end up waiting longer than the user wanted.

I'm not sure how to tackle that: either we accept that an incoming signal will make the wait longer, or we fix it by properly decrementing the timeout (which will complicate things a bit, especially for cross-platform time querying - but see issue9079 which might help us).
msg110110 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-07-12 16:45
Accepting that the timeout is not perfect in the face of signals as a caveat is fine (document the possibility) and can be improved later.
msg110156 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-07-13 04:12
Alternatively, do you think it would be better to ignore interrupts when a timeout is passed?  If a timeout is passed, the lock acquire will eventually fail in a deadlock situation, and the signal will be handled in the eval loop.

However, if the timeout is sufficiently long, this is still a problem.

I'd prefer to do that or use gettimeofday from _time than leave this as is.
msg110653 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-07-18 15:04
Waiting until the portability hacks for gettimeofday make it into core Python.
msg113786 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-13 15:26
The portability API is now available (see Include/pytime.h).
msg113941 - (view) Author: Reid Kleckner (rnk) (Python committer) Date: 2010-08-15 04:51
Added a patch that adds support for recomputing the timeout, plus a test for it.

Can this still make it into 3.2, or is it too disruptive at this point in the release process?
msg113953 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-15 12:51
> Added a patch that adds support for recomputing the timeout, plus a test for it.
> 
> Can this still make it into 3.2, or is it too disruptive at this point
> in the release process?

No problem at this point, we're not yet in beta phase.
I haven't looked at the patch itself, but thank you.
msg124088 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-12-15 22:59
Committed in r87292. Thank you for doing this!
msg125715 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-01-07 21:45
The test fails on FreeBSD 6.2 (x86 FreeBSD py3k buildbot) since r87292 => #10720. I reopen the issue.
msg125719 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-07 21:54
Well, since #10720 exists, while do you reopen this one?
History
Date User Action Args
2013-11-07 12:58:04Yuri.Bochkarevsetnosy: + Yuri.Bochkarev
2011-02-16 15:40:23pitrousetstatus: open -> closed
nosy: gregory.p.smith, pitrou, haypo, jyasskin, rnk, hobb0001
resolution: fixed
2011-01-07 21:54:50pitrousetnosy: gregory.p.smith, pitrou, haypo, jyasskin, rnk, hobb0001
messages: + msg125719
2011-01-07 21:45:50hayposetstatus: closed -> open

nosy: + haypo
messages: + msg125715

resolution: fixed -> (no value)
2010-12-15 22:59:55pitrousetstatus: open -> closed
nosy: gregory.p.smith, pitrou, jyasskin, rnk, hobb0001
messages: + msg124088

resolution: accepted -> fixed
stage: commit review -> committed/rejected
2010-12-15 20:38:39pitrousetnosy: gregory.p.smith, pitrou, jyasskin, rnk, hobb0001
dependencies: - Make gettimeofday available in time module
2010-12-15 20:38:33pitrousetassignee: pitrou ->
nosy: gregory.p.smith, pitrou, jyasskin, rnk, hobb0001
2010-08-15 12:52:00pitrousetmessages: + msg113953
2010-08-15 04:51:41rnksetfiles: + lock-interrupt-v4.diff

messages: + msg113941
2010-08-13 15:26:02pitrousetmessages: + msg113786
2010-07-18 15:04:11rnksetdependencies: + Make gettimeofday available in time module
messages: + msg110653
2010-07-13 04:12:08rnksetmessages: + msg110156
2010-07-12 16:45:52gregory.p.smithsetmessages: + msg110110
2010-07-12 16:08:42pitrousetmessages: + msg110105
2010-07-11 13:36:01pitrousetassignee: pitrou
resolution: accepted
messages: + msg110007
stage: commit review
2010-07-10 20:57:58rnksetmessages: + msg109935
2010-07-10 20:32:07rnksetfiles: + lock-interrupt.diff

messages: + msg109932
2010-07-10 19:43:55pitrousetmessages: + msg109915
2010-07-10 17:28:17rnksetfiles: + lock-interrupt.diff

messages: + msg109898
2010-07-10 16:13:52pitrousetmessages: + msg109884
versions: - Python 3.1
2010-07-10 15:47:46rnksetfiles: + lock-interrupt.diff
keywords: + patch
messages: + msg109883
2010-05-29 11:44:42pitrousetmessages: + msg106715
2010-05-29 02:59:28rnksetmessages: + msg106705
2010-05-28 19:16:20pitrousetmessages: + msg106685
2010-05-28 17:40:38pitrousetnosy: + gregory.p.smith, rnk, jyasskin, pitrou

messages: + msg106680
versions: + Python 3.2
2010-05-28 16:48:10hobb0001create