Repository navigation
queue.Queue() is not reentrant, so signals and GC can cause deadlocks #59181
Description
| BPO | 14976 |
|---|---|
| Nosy | @gvanrossum, @tim-one, @rhettinger, @gpshead, @ncoghlan, @pitrou, @serhiy-storchaka, @1st1, @applio |
| PRs | |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2018-01-15.23:27:50.989>
created_at = <Date 2012-06-01.08:43:03.911>
labels = ['3.7', 'type-bug', 'library']
title = 'queue.Queue() is not reentrant, so signals and GC can cause deadlocks'
updated_at = <Date 2018-01-17.01:04:26.008>
user = 'https://bugs.python.org/JohanAR'bugs.python.org fields:
activity = <Date 2018-01-17.01:04:26.008>
actor = 'gregory.p.smith'
assignee = 'none'
closed = True
closed_date = <Date 2018-01-15.23:27:50.989>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2012-06-01.08:43:03.911>
creator = 'JohanAR'
dependencies = []
files = ['25786', '25787', '46806', '47092']
hgrepos = []
issue_num = 14976
keywords = ['patch']
message_count = 51.0
messages = ['162062', '162065', '162069', '162076', '211436', '275181', '275377', '275404', '275421', '275485', '291764', '300421', '300422', '300455', '300478', '300489', '300491', '300499', '300504', '300505', '300509', '300510', '300511', '300532', '300567', '300574', '300575', '300577', '300581', '300723', '300825', '301170', '301173', '301175', '301176', '301207', '301299', '301321', '301322', '301378', '301379', '301380', '301383', '301386', '301409', '310014', '310025', '310026', '310109', '310111', '310125']
nosy_count = 15.0
nosy_names = ['gvanrossum', 'tim.peters', 'rhettinger', 'gregory.p.smith', 'ncoghlan', 'pitrou', 'zzzeek', 'python-dev', 'sbt', 'serhiy.storchaka', 'JohanAR', 'yselivanov', 'itamarst', 'davin', 'Catalin Patulea']
pr_nums = ['3346']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14976'
versions = ['Python 3.7']
Activity
JohanAR commentedon Jun 1, 2012
Python 2.7.3 (default, Apr 20 2012, 22:39:59)
[GCC 4.6.3] on linux2
If a signal handler calls Queue.PriorityQueue.put and a second signal is received while one is already being processed, neither of the calls to put will terminate.
Highly dependent on timing so it might be difficult to reproduce.
JohanAR commentedon Jun 1, 2012
Start queue_deadlock.py in one terminal and note the PID it prints.
Compile queue_sendint.c in another terminal and execute it with previous PID as only argument.
If the bug is triggered, nothing will print in the python terminal window when you press Enter. To terminate the application you can press Ctrl-\
sbt commentedon Jun 1, 2012
I don't think there is anything special about PriorityQueue.
There is a similar concerning the use of the Python implementation of RLock in signal handlers -- see http://bugs.python.org/issue13697.
Maybe the signal handler should temporarily mask or ignore SIGINT while it runs.
JohanAR commentedon Jun 1, 2012
I did read some more on the subject and it seems like using locks with interrupts is in general a very difficult problem and not limited to Python.
Don't know if this is considered common knowledge among programmers or if it would be useful with at least a warning on the signal manual page.
itamarst commentedon Feb 17, 2014
This is not specifically a signal issue; it can happen with garbage collection as well if you have a Queue.put that runs in __del__ or a weakref callback function.
This can happen in real code. In my case, a thread that reads log messages from a Queue and writes them to disk. The thread putting log messages into the Queue can deadlock if GC happens to cause a log message to be written right after Queue.put() acquired the lock.
(see itamarst/crochet#25).
[-]Queue.PriorityQueue() is not interrupt safe[/-][+]queue.Queue() is not reentrant, so signals and GC can cause deadlocks[/+]zzzeek commentedon Sep 8, 2016
SQLAlchemy suffered from this issue long ago as we use a Queue for connections, which can be collected via weakref callback and sent back to put(), which we observed can occur via gc. For many years (like since 2007 or so) we've packaged a complete copy of Queue.Queue with an RLock inside of it to work around it.
I'm just noticing this issue because I'm writing a new connection pool implementation that tries to deal with this issue a little differently (not using Queue.Queue though).
rhettinger commentedon Sep 9, 2016
Guido opposes having us going down the path of using an RLock and altering the queue module to cope with reentrancy.
In the case of mixing signals with threads, we all agree with Johan Aires Rastén that "using locks with interrupts is in general a very difficult problem and not limited to Python", nor is it even limited to the queue module.
I believe that the usual technique for mixing signals and threads is to have the signal handler do the minimal work necessary to record the event without interacting with the rest of the program. Later, another thread can act on the recorded signal but can do so with normal use of locks so that code invariants are not violated. This design makes reasoning about the program more tractable.
python-dev commentedon Sep 9, 2016
New changeset 137806ca59ce by Gregory P. Smith in branch '3.5':
Add a note about queue not being safe for use from signal handlers.
https://hg.python.org/cpython/rev/137806ca59ce
New changeset 4e15e7618715 by Gregory P. Smith in branch 'default':
Add a note about queue not being safe for use from signal handlers.
https://hg.python.org/cpython/rev/4e15e7618715
zzzeek commentedon Sep 9, 2016
yep, that's what im doing in my approach. though longer term thing, I noticed it's very hard to find documentation on exactly when gc might run. E.g. would it ever run if I did something innocuous, like "self.thread_id = None" (probably not). Just wasn't easy to get that answer.
python-dev commentedon Sep 9, 2016
New changeset 8c00cbbd3ff9 by Raymond Hettinger in branch '3.5':
bpo-14976: Note that the queue module is not designed to protect against reentrancy
https://hg.python.org/cpython/rev/8c00cbbd3ff9
itamarst commentedon Apr 16, 2017
This bug was closed on the basis that signals + threads don't interact well. Which is a good point.
Unfortunately this bug can happen in cases that have nothing to do with signals. If you look at the title and some of the comments it also happens as a result of garbage collection: in
__del__methods and weakref callbacks.Specifically, garbage collection can happen on any bytecode boundary and cause reentrancy problems with Queue.
The attached file is an attempt to demonstrate this: it runs and runs until GC happens and then deadlocks for me (on Python 3.6). I.e. it prints GC! and after that no further output is printed and no CPU usage reported.
Please re-open this bug: if you don't want to fix signal case that's fine, but the GC case is still an issue.
31 remaining items
pitrou commentedon Sep 5, 2017
I don't mind someone to reimplement a full-fledged Queue in C. As for me, I am currently implementing a SimpleQueue in C that's reentrant and has only the most basic functionality.
gpshead commentedon Sep 5, 2017
FYI - here is an appropriately licensed (WTFPL) very simple lock-free queue implemented in C: https://github.com/darkautism/lfqueue
pitrou commentedon Sep 5, 2017
Le 05/09/2017 à 23:21, Gregory P. Smith a écrit :
Looks like it uses busy-waiting inside its get() equivalent.
Also we would need a portable version of the atomic instructions used
there (e.g. __sync_bool_compare_and_swap) :-)
gpshead commentedon Sep 5, 2017
yeah, there are others such as https://www.liblfds.org/ that seem better from that standpoint (gcc, and microsoft compiler support - I'm sure clang is fine as well - anything gcc can do they can do). Ensuring they're supported across build target platforms (all the hardware technically has the ability) is the fun part if we're going to go that route.
pitrou commentedon Sep 5, 2017
One tangential note about a potential full-fledged Queue in C: the pure Python implementation is fair towards consumers, as it uses a threading.Condition which is itself fair. Achieving the same thing in C may significantly complicate the implementation. Raymond would have to decide whether it's an important property to keep.
The SimpleQueue class, being a separate API, is not tied by this constraint.
rhettinger commentedon Sep 5, 2017
To handle the logging.handlers.QueueHandler case, the SimpleQueue needs a put_nowait() method (see line 1959 of Lib/logging/handlers.py.
[Mike Bayer]
The answer I got from the other coredevs is that GC can trigger whenever a new GCable object is allocated (pretty much any container or any pure python object).
rhettinger commentedon Sep 6, 2017
Just for the record, Guido explained his aversion to RLocks. Roughly: 1) RLocks are slower and more complex 2) It is difficult to correctly write code that can survive reentrancy, so it is easy fool yourself into believing you've written correct code 3) Getting a deadlock with a regular lock is a better outcome than having the invariants subtly violated, 4) Deadlocks have a more clear-cut failure mode and are easier to debug.
Guido also explained that he favored some sort of minimal queue class even if it might not handle all possible gc/weakref/signal/del induced issues. Essentially, he believes there is some value in harm reduction by minimizing the likelihood of a failure. The will help sysadmins who already have a practice of doing monitoring and occasional restarts as a way of mitigating transient issues.
pitrou commentedon Jan 15, 2018
Hi all,
The PR has been ready for quite some time now. Raymond posted some review comments back in September, which I addressed by making the requested changes.
If someone wants to add their comments, now is the time. Otherwise I plan to merge the PR sometimes soon, so that it gets in before 3.7 beta.
pitrou commentedon Jan 15, 2018
New changeset 94e1696 by Antoine Pitrou in branch 'master':
bpo-14976: Reentrant simple queue (bpo-3346)
94e1696
pitrou commentedon Jan 15, 2018
Ok, there has been enough reviews in the last four months :-) This is now merged.
gpshead commentedon Jan 16, 2018
Catalin has been examining code... switching concurrent.futures.thread to use SimpleQueue instead of Queue is probably a good idea as the queues in there get used from weakref callbacks.
pitrou commentedon Jan 16, 2018
Could you open a new issue for it?
gpshead commentedon Jan 17, 2018
https://bugs.python.org/issue32576 filed for that