[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Gregory P. Smith added the comment: https://bugs.python.org/issue32576 filed for that -- ___ Python tracker ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Could you open a new issue for it? -- ___ Python tracker ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Gregory P. Smith added the comment: 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. -- ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Ok, there has been enough reviews in the last four months :-) This is now merged. -- resolution: -> fixed stage: needs patch -> resolved status: open -> closed ___ Python tracker

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: New changeset 94e1696d04c65e19ea52e5c8664079c9d9aa0e54 by Antoine Pitrou in branch 'master': bpo-14976: Reentrant simple queue (#3346) https://github.com/python/cpython/commit/94e1696d04c65e19ea52e5c8664079c9d9aa0e54 --

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Change by Catalin Patulea : -- nosy: +Catalin Patulea ___ Python tracker ___ ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Raymond Hettinger added the comment: 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)

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Raymond Hettinger added the comment: To handle the logging.handlers.QueueHandler case, the SimpleQueue needs a put_nowait() method (see line 1959 of Lib/logging/handlers.py. [Mike Bayer] > I noticed it's very hard to find documentation on exactly when > gc might run. The answer I got from

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: 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.

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Gregory P. Smith added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Le 05/09/2017 à 23:21, Gregory P. Smith a écrit : > > FYI - here is an appropriately licensed (WTFPL) very simple lock-free queue > implemented in C Looks like it uses busy-waiting inside its get() equivalent. Also we would need a portable version of the

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Gregory P. Smith added the comment: FYI - here is an appropriately licensed (WTFPL) very simple lock-free queue implemented in C: https://github.com/darkautism/lfqueue -- nosy: +gregory.p.smith ___ Python tracker

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Changes by Antoine Pitrou : -- pull_requests: +3357 ___ Python tracker ___ ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: 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. -- ___ Python tracker

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: > Would it make sense get() and put() to add gc.disable() and gc.enable() > whenever GC is already enabled? That doesn't sound very nice if some thread is waiting on a get() for a very long time (which is reasonable if you have a thread pool that's only used

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Raymond Hettinger added the comment: > I can't think of a reason why deque would need to release the GIL. Yes, in deque.append(item) and deque.popleft() are atomic. Likewise list.append() and list.pop() are atomic. The heappush() and heappop() operations aren't as fortunate since they call

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Guido van Rossum added the comment: Oh well. While it is undoubtedly useful I wish we had had more experience and factored the API differently. Ditto for the maxsize=N feature. So, while it's not too late, perhaps we should indeed follow Antoine's advice and implement a different queue that

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Raymond Hettinger added the comment: [Guido] > Why was task management ever added? See http://bugs.python.org/issue1455676 Problem being solved: How can a requestor of a service get notified when that service is complete given that the work is being done by a daemon thread that never

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Tim Peters added the comment: [Guido] > Why was task management ever added? Raymond published a "joinable" queue class as a recipe here: http://code.activestate.com/recipes/475160-taskqueue/ and later folded it into the standard Python queue. So the usual answer applies: "it was easy to

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Guido van Rossum added the comment: Agreed; the Queue class has a bunch of rarely used functionality rolled in... Why was task management ever added? -- ___ Python tracker

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Just a random thought: if there was a SimpleQueue class with very basic functionality (only FIFO, only get(), put() and empty(), no size limit, no task management), it would be easier to make it reentrant using C. (FTR, multiprocessing also has a light-weight

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Raymond Hettinger added the comment: [Antoine Pitrou] > So perhaps we need C code after all. This matches my experience with functools.lru_cache() where I used an RLock() to handle reentrancy. That by itself was insufficient. I also had to make otherwise unnecessary variable assignments to

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: After experimenting a bit more with this approach, I now realize that the case where a get() is waiting and gets interrupted by a put() call is not handled properly: there is no obvious way for the get() call to realize (when the interruption finishes) that

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Le 19/08/2017 à 12:09, Nick Coghlan a écrit : > > Would it be feasible to change the behaviour of non-reentrant locks such that: > > 1. They *do* keep track of the owning thread Yes. > 2. Trying to acquire them again when the current thread already has them

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Nick Coghlan added the comment: Would it be feasible to change the behaviour of non-reentrant locks such that: 1. They *do* keep track of the owning thread 2. Trying to acquire them again when the current thread already has them locked raises RuntimeError instead of deadlocking the way it does

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Oh and: Le 18/08/2017 à 23:26, Guido van Rossum a écrit : > > I can't say I understand all of Antoine's patch, but it's probably okay to do > it this way; however I would rather see if we can add _is_owned() to Lock, > assuming it can be implemented using

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Le 18/08/2017 à 23:26, Guido van Rossum a écrit : > > IIUC the end result would be a Queue whose put() works from signal handlers, > GC callbacks and __del__, as long as it's unbounded, right? Yes. > And when it *is* bounded, it will give a decent message if

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Nick Coghlan added the comment: +1 for treating Queue.put() specifically as the case to be handled, as that's the mechanism that can be used to *avoid* running complex operations directly in __del__ methods and weakref callbacks. For testing purposes, the current deadlock can be reliably

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Guido van Rossum added the comment: Given the date from that comment I assume that I told Raymond this during the 2016 core sprint. I can't recall that conversation but I am still pretty worried about using an RLock. (What if someone slightly more insane decides to call get() from inside a GC

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Le 18/08/2017 à 17:37, Yury Selivanov a écrit : > > Is it guaranteed that the GC will happen in the same thread that is holding > the lock? By design, if it's called from a different thread, Queue will cope fine: __del__ implicitly calls RLock.acquire which,

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Yury Selivanov added the comment: > Here is a pure Python PoC patch that allows unbounded Queue and LifoQueue to > have reentrant put(). Is it guaranteed that the GC will happen in the same thread that is holding the lock? IOW will RLock help with all GC/__del__ deadlocking scenarios?

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Also, to elaborate a bit, I don't think we should aim to make Queue fully reentrant, as that would be extremely involved. I think we can settle on the simpler goal of making put() reentrant for unbounded LIFO and FIFO queues, which is what most people care

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Guido, apparently you are opposed to the Queue implementation using a RLock instead of a Lock, according to Raymond in https://bugs.python.org/issue14976#msg275377. I find this a bit surprising, so could you confirm it yourself? -- nosy: +gvanrossum

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: I'll believe it when Guido chimes in and actually says so himself. Otherwise, I am skeptical Guido cares a lot about whether the internal implementation of Queue uses a Lock, a RLock or whatever else. -- ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

mike bayer added the comment: > Here is a pure Python PoC patch that allows unbounded Queue and LifoQueue to > have reentrant put(). per http://bugs.python.org/msg275377 guido does not want an RLock here. -- ___ Python tracker

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: `unfinished_tasks` is not a public attribute AFAIK (it's not documented). The change is necessary: you cannot increment unfinished_tasks in reentrant put(), since incrementing in pure Python is not atomic. So the incrementation is moved to get(), which

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Serhiy Storchaka added the comment: Are all changes are necessary for this issue or some of them are just an optimization? The patch changes the semantic of public attribute unfinished_tasks. Seems that old unfinished_tasks is new unfinished_tasks + _qsize(). --

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Here is a pure Python PoC patch that allows unbounded Queue and LifoQueue to have reentrant put(). -- keywords: +patch nosy: +serhiy.storchaka Added file: http://bugs.python.org/file47092/q_reentrancy.patch ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Antoine Pitrou added the comment: Using any kind of potentially-blocking synchronization primitive from __del__ or weakref callback is indeed a bug waiting for happen. I agree non-trivial cases can be hard to debug, especially when people don't expect that kind of cause. It would be ok to

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Yury Selivanov added the comment: An idea from the blog post: if we rewrite queue in C it will use the GIL as a lock which will fix this particular bug. I can make a patch. -- status: closed -> open ___ Python tracker

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Nick Coghlan added the comment: Itamar wrote up a post describing the GC variant of this problem in more detail: https://codewithoutrules.com/2017/08/16/concurrency-python/ In particular, he highlighted a particularly nasty action-at-a-distance variant of the deadlock where: 1. Someone

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Changes by Yury Selivanov : -- nosy: +yselivanov ___ Python tracker ___ ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Itamar Turner-Trauring added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Roundup Robot added the comment: New changeset 8c00cbbd3ff9 by Raymond Hettinger in branch '3.5': Issue 14976: Note that the queue module is not designed to protect against reentrancy https://hg.python.org/cpython/rev/8c00cbbd3ff9 -- ___ Python

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

mike bayer added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Roundup Robot added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Raymond Hettinger added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

mike bayer added the comment: 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

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Changes by Raymond Hettinger raymond.hettin...@gmail.com: -- nosy: +rhettinger, tim.peters ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue14976 ___

[issue14976] queue.Queue() is not reentrant, so signals and GC can cause deadlocks

Itamar Turner-Trauring added the comment: 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