Repository navigation
asyncio doesn't warn if a task is destroyed during its execution #65362
Description
| BPO | 21163 |
|---|---|
| Nosy | @gvanrossum, @pitrou, @vstinner, @giampaolo, @1st1, @gvanrossum, @mpaolini |
| 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 2014-07-16.16:57:08.632>
created_at = <Date 2014-04-05.21:10:31.905>
labels = ['type-bug', 'library', 'expert-asyncio']
title = "asyncio doesn't warn if a task is destroyed during its execution"
updated_at = <Date 2014-09-20.21:15:28.592>
user = 'https://bugs.python.org/richardkiss'bugs.python.org fields:
activity = <Date 2014-09-20.21:15:28.592>
actor = 'gvanrossum'
assignee = 'none'
closed = True
closed_date = <Date 2014-07-16.16:57:08.632>
closer = 'vstinner'
components = ['Library (Lib)', 'asyncio']
creation = <Date 2014-04-05.21:10:31.905>
creator = 'richard.kiss'
dependencies = []
files = ['34741', '35691', '35781', '36405', '36408', '36413']
hgrepos = []
issue_num = 21163
keywords = ['patch']
message_count = 37.0
messages = ['215633', '215638', '215639', '215640', '215642', '215646', '215647', '220986', '221010', '221096', '221375', '221387', '221470', '221495', '221503', '221508', '221573', '221574', '221578', '221580', '221956', '221957', '222699', '223230', '223232', '223233', '225493', '225497', '225498', '225499', '225500', '225504', '225520', '225522', '227173', '227176', '227177']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'pitrou', 'vstinner', 'giampaolo.rodola', 'python-dev', 'yselivanov', 'Guido.van.Rossum', 'richard.kiss', 'Richard.Kiss', 'mpaolini']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21163'
versions = ['Python 3.4', 'Python 3.5']
Activity
RichardKiss commentedon Apr 5, 2014
Some tasks created via asyncio are vanishing because there is no reference to their resultant futures.
This behaviour does not occur in Python 3.3.3 with asyncio-0.4.1.
Also, doing a gc.collect() immediately after creating the tasks seems to fix the problem.
Attachment also available at https://gist.github.com/richardkiss/9988156
[-]asyncio Task Possibly Incorrectly Garbage Collected[/-][+]asyncio task possibly incorrectly garbage collected[/+]gvanrossum commentedon Apr 5, 2014
Ouch. That example is very obfuscated -- I fail to understand what it is trying to accomplish. Running it I see that it always prints 100 for the count with 3.3 or DO_CG on; for me it prints 87 with 3.4 an DO_GC off. But I wouldn't be surprised if the reason calling do.collect() "fixes" whatever issue you have is that it causes there not to be any further collections until the next cycle through that main loop, and everything simply runs before being collected. But that's just a theory based on minimal understanding of the example.
I'm not sure that tasks are *supposed* to stay alive when there are no references to them. It seems that once they make it past a certain point they keep themselves alive.
One more thing: using a try/except I see that the "lost" consumers all get a GeneratorExit on their first iteration. You might want to look into this. (Sorry, gotta run, wanted to dump this.)
RichardKiss commentedon Apr 5, 2014
I agree it's confusing and I apologize for that.
Background:
This multiplexing pattern is used in pycoinnet, a bitcoin client I'm developing at <https://github.com/richardkiss/pycoinnet\>. The BitcoinPeerProtocol class multiplexes protocol messages into multiple asyncio.Queue objects so each interested listener can react. An example listener is in pycoinnet.helpers.standards.install_pong_manager, which looks for "ping" messages and sends "pong" responses. When the peer disconnects, the pong manager sees a None (to indicate EOF), and it exits. The return value is uninteresting, so no reference to the Task is kept.
My client is in late alpha, and mostly works, but when I tried it on Python 3.4.0, it stopped working and I narrowed it down to this.
I'm not certain this behaviour is incorrect, but it's definitely different from 3.3.3, and it seems odd that a GC cycle BEFORE additional references can be made would allow it to work.
gvanrossum commentedon Apr 5, 2014
Most likely your program is simply relying on undefined behavior and the right way to fix it is to keep strong references to all tasks until they self-destruct.
RichardKiss commentedon Apr 5, 2014
I'll investigate further.
RichardKiss commentedon Apr 6, 2014
You were right: adding a strong reference to each Task seems to have solved the original problem in pycoinnet. I see that the reference to the global lists of asyncio.tasks is a weakset, so it's necessary to keep a strong reference myself.
This does seem a little surprising. It can make it trickier to create a task that is only important in its side effect. Compare to threaded programming: unreferenced threads are never collected.
For example:
In theory, the "some_coroutine" task can be collected, preventing "some_completion_callback" from ever being called. While technically correct, it does seem surprising.
(I couldn't get this to work in a simple example, although I did get it to work in a complicated example.)
Some change between 3.3 and 3.4 means garbage collection is much more aggressive at collecting up unreferenced tasks, which means broken code, like mine, that worked in 3.3 fails in 3.4. This may trip up other early adopters of tulip.
Maybe adding a "do_not_collect=True" flag to asyncio.async or asyncio.Task, which would keep a strong reference by throwing it into a singleton set (removing it as a future callback) would bring attention to this subtle issue. Or displaying a warning in debug mode when a Task is garbage-collected before finishing.
Thanks for your help.
gvanrossum commentedon Apr 6, 2014
Thanks for understanding.
It's definitely subtle: there is also some code in asyncio that logs an error when a Future holding an exception becomes unreachable before anyone has asked for it; this has been a valuable debugging tool, and it depends on *not* holding references to Futures.
Regarding the difference between Python 3.3 and 3.4, I don't know the exact reason, but GC definitely gets more precise with each new revision, and there are also some differences in how exactly the debug feature I just mentioned is implemented (look for _tb_logger in asyncio/futures.py).
OTOH it does seem a little odd to just GC a coroutine that hasn't exited yet, and I'm not 100% convinced there *isn't* a bug here. The more I think about it, the more I think that it's suspicious that it's always the *first* iteration that gets GC'ed. So I'd like to keep this open as a reminder.
Finally, I'm not sure the analogy with threads holds -- a thread manages OS resources that really do have to be destroyed explicitly, but that's not so for tasks, and any cleanup you do need can be handled using try/finally. (In general drawing analogies between threads and asyncio tasks/coroutines is probably one of the less fruitful lines of thinking about the latter; there are more differences than similarities.)
vstinner commentedon Jun 19, 2014
Ok, I agree that this issue is very tricky :-)
The first problem in asyncio-gc-issue.py is that the producer keeps *weak* references to Queue object, so the Queue objects are quickly destroyed, especially if gc.collect() is called explicitly.
When "yield from queue.get()" is used in a task, the task is paused. The queue creates a Future object and the task registers its _wakeup() method into the Future object.
When the queue object is destroyed, the internal future object (used by the get() method) is destroyed too. The last reference to the task was in this future object. As a consequence, the task is also destroyed.
While there is a bug in asyncio-gc-issue.py, it's very tricky to understand it and I think that asyncio should help developers to detect such bugs.
I propose attached patch which emits a warning if a task is destroyed whereas it is not done (its status is still PENDING). I wrote a unit test which is much simpler than asyncio-gc-issue.py. Read the test to understand the issue. I added many comments to explain the state.
--
My patch was written for Python 3.4+: it adds a destructor to the Task class, and we cannot add a destructor in Future objects because these objects are likely to be part of reference cycles. See the following issue which proposes a fix:
https://code.google.com/p/tulip/issues/detail?id=155
Using this fix for reference cycle, it may be possible to emit also the log in Tulip (Python 3.3).
[-]asyncio task possibly incorrectly garbage collected[/-][+]asyncio doesn't warn if a task is destroyed during its execution[/+]16 remaining items
python-dev commentedon Jul 16, 2014
New changeset e4fe6706b7b4 by Victor Stinner in branch '3.4':
Issue bpo-21163: Fix "destroy pending task" warning in test_wait_errors()
http://hg.python.org/cpython/rev/e4fe6706b7b4
New changeset a627b23f57d4 by Victor Stinner in branch 'default':
(Merge 3.4) Issue bpo-21163: Fix "destroy pending task" warning in test_wait_errors()
http://hg.python.org/cpython/rev/a627b23f57d4
vstinner commentedon Jul 16, 2014
Ok, I fixed the last warnings emitted in unit tests ran in debug mode. I close the issue.
mpaolini commentedon Aug 18, 2014
I finally wrapped my head around this. I wrote a (simpler) script to get a better picture.
What happens
-------------
When a consumer task is first istantiated, the loop holds a strong reference to it (_ready)
Later on, as the loop starts, the consumer task is yielded and it waits on an unreachable future. The last strong ref to it is lost (loop._ready).
It is not collected immediately because it just created a reference loop
(task -> coroutine -> stack -> future -> task) that will be broken only at task completion.
gc.collect() called *before* the tasks are ever run has the weird side effect of moving the automatic gc collection forward in time.
Automatic gc triggers after a few (but not all) consumers have become unreachable, depending on how many instructions were executed before running the loop.
gc.collect() called after all the consumers are waiting on the unreachable future reaps all consumer tasks as expected. No bug in garbage collection.
Yielding from asyncio.sleep() prevents the consumers from being
collected: it creates a strong ref to the future in the loop.
I suspect also all network-related asyncio coroutines behave this way.
Summing up: Tasks that have no strong refs may be garbage collected unexpectedly or not at all, depending on which future they yield to. It is very difficult to debug and undestand why these tasks disappear.
Side note: the patches submitted and merged in this issue do emit the relevant warnings when PYTHONASYNCIODEBUG is set. This is very useful.
Proposed enhanchements
----------------------
Document that you should always keep strong refs to tasks or to futures/coroutines the tasks yields from. This knowledge is currently passed around the brave asyncio users like oral tradition.
Alternatively, keep strong references to all futures that make it through Task._step. We are already keeping strong refs to some of the asyncio builtin coroutines (
asyncio.sleepis one of those). Also, we do keep strong references to tasks that are ready to be run (the ones that simplyyieldor the ones that have not started yet)If you also think 1. or 2. are neeed, let me know and I'll try cook a patch.
Sorry for the noise
gvanrossum commentedon Aug 18, 2014
I'm all in favor of documenting that you must keep a strong reference to a
task that you want to keep alive. I'm not keen on automatically keep all
tasks alive, that might exacerbate leaks (which are by definition hard to
find) in existing programs.
On Mon, Aug 18, 2014 at 7:20 AM, Marco Paolini <report@bugs.python.org>
wrote:
mpaolini commentedon Aug 18, 2014
Asking the user to manage strong refs is just passing the potential
leak issue outside of the standard library. It doesn't really solve anything.
If the user gets the strong refs wrong he can either lose tasks or
leak memory.
If the standard library gets it right, both issues are avoided.
gvanrossum commentedon Aug 18, 2014
So you are changing your mind and withdrawing your option #1.
I don't have the time to really dig deeply into the example app and what's
going on. If you want to help, you can try to come up with a patch (and it
should have good unit tests).
I'll be on vacation most of this week.
On Mon, Aug 18, 2014 at 9:17 AM, Marco Paolini <report@bugs.python.org>
wrote:
mpaolini commentedon Aug 18, 2014
I think option #1 (tell users to keep strong refs to tasks) is
OK but option #2 is better.
Yes, I changed my mind ;)
mpaolini commentedon Aug 18, 2014
Submitted a first stab at #2. Let me know what you think.
If this works we'll have to remove the test_gc_pending test and then maybe even the code that now logs errors when a pending task is gc'ed
vstinner commentedon Aug 19, 2014
I don't understand how keeping a strong refrence would fix anything. You
only provided one example (async-gc-bug.py) which uses Queue objects but
keep weak references to them. Keeping strong references to tasks is not the
right fix. You must keep strong references to queues. If a queue is
destroyed, how can you put an item into it? Otherwise, the task will wait
forever. Keeping a strong refrence to the task just hides the bug. Or I
missed something.
I dislike the idea of keeping strong references to tasks, it may create
even more reference cycles. We already have too many cycles with exceptions
stored in futures (in tasks).
The current unit test uses low level functions to remove a variable using a
frame object. Can you provide an example which shows the bug without using
low level functions?
mpaolini commentedon Aug 19, 2014
The original asyncio-gc-issue.py wasn't written by me, and yes, as you say it does have the reference bug you describe. I argue that bug shouldn't cause tasks to die: it should rather limit (as gc proceeds) the number of queues available to the producer in the WeakSet() and leaving alive all consumer waiting on an unreachable queue.
Please look at my test2.py or even better test3.py for a simpler example.
Note that in my issue_22163_patch_0.diff I only keep strong refs to futures a task is waiting on. Just as asyncio is already doing with asyncio.sleep() coroutine.
I dislike the idea of randomly losing tasks.
I also dislike the idea of forcing the user to manage strong refs to its tasks. All 3rd party libraries will have to invent their own way and it will lead to even more leaks/cycles very hard to debug.
Not just exceptions: everytime a task is yielding on a future asyncio creates a reference cycle.
My issue_22163_patch_0.diff only clears references by setting variables to
None. No low level stuff needed.My test2.py example script also doesn't use any low level stuff
I just uploaded test3.py with a simpler (and possibly more realistic) example.
mpaolini commentedon Sep 20, 2014
Sorry for keeping this alive.
Take a look at the
wait_for.pyjust submitted in the unrelated bpo-22448: no strong refs to the tasks are kept. Tasks remain alive only because they are timers and the event loop keeps strong ref.Do you think my proposed patch is OK? Sould I open a new issue?
gvanrossum commentedon Sep 20, 2014
I'm not sure how that wait_for.py example from bpo-2116 relates to this issue -- it seems to demonstrate the opposite problem (tasks are kept alive even though they are cancelled).
Then again I admit I haven't looked deeply into the example (though I am sympathetic with the issue it purports to demonstrate).
gvanrossum commentedon Sep 20, 2014
(Whoops meant to link to bpo-22448.)