Properly await coroutines registered with app.on_shutdown #4641
Conversation
to not mix up the loops
to make sure it's not lost when resetting globals between test runs
to ensure all tasks and coroutines are really done before entering the next test
to better identify pending tasks and where they come from
… action is having to few resources
|
The complexity of this PR has grown as we speak (or code?). I noticed that it currently contains 51 commits and a lot of changes over quite a lot of files, which makes it challenging for anyone to review and assess the overall impact. It may be helpful to squash commits. Separate some fixes into another PR seems like another idea, with the added benefit of being able to see which changes maintain the passing test case while which breaks our tests. However, this isn't strictly necesary, since Rodja and Falko still make for a killer combo as they can talk offline. Mark ignore to this concern by reacting 👀. Though I will say, regardless, it's too hard for me, I'm out as always. 😅 |
There was a problem hiding this comment.
Ok, we're almost there. But right now we think collecting callables in functions_awaited_on_shutdown causes a potential memory leak, e.g. if the user decorates a function which is defined within a page function. So we have to either prune efficiently, work with weak references or find a different way to "mark" functions as "await on shutdown".
This reverts commit 27a0f0d.
# Conflicts: # poetry.lock # pyproject.toml
There was a problem hiding this comment.
I wonder if we really need _should_await_on_shutdown. See my corresponding comment below.
Apart from that I tried to use the new decorator with the following snippet:
async def test1():
print('test1 start')
await asyncio.sleep(3)
print('test1 end')
@background_tasks.await_on_shutdown
async def test2():
print('test2 start')
await asyncio.sleep(3)
print('test2 end')
ui.button('Test1', on_click=lambda: background_tasks.create(test1()))
ui.button('Test2', on_click=lambda: background_tasks.create(test2()))
ui.button('Shutdown', on_click=app.shutdown)But when quickly clicking "Test1"-"Test2"-"Shutdown", both tasks seem to be cancelled before the second print statement is executed. Am I doing anything wrong?
| while running_tasks or lazy_tasks_running: | ||
| tasks = set(running_tasks) | set(lazy_tasks_running.values()) | ||
| for task in tasks: | ||
| if not task.done() and not task.cancelled() and not _should_await_on_shutdown(task): |
There was a problem hiding this comment.
| if not task.done() and not task.cancelled() and not _should_await_on_shutdown(task): | |
| if not task.done() and not task.cancelled() and task.get_coro() not in functions_awaited_on_shutdown: |
Is this possible?
| async def teardown() -> None: | ||
| """Cancel all running tasks and coroutines on shutdown. (For internal use only.)""" | ||
| while running_tasks or lazy_tasks_running: | ||
| tasks = set(running_tasks) | set(lazy_tasks_running.values()) |
There was a problem hiding this comment.
| tasks = set(running_tasks) | set(lazy_tasks_running.values()) | |
| tasks = running_tasks | set(lazy_tasks_running.values()) |
running_tasks is already a set.
This PR started with the intent to fix #4592 which reported unawaited coroutines when running pytests. It turns out, the warning lead down a rabbit hole of problems. Here is a list of the major findings and fixes:
app.on_shutdownhave not been awaited properlybackground_taskswhere not canceled on shutdownoutbox.loopdid not exit when receiving a cancel commandasyncio.wait_formisses cancel commands in certain conditions and must be fixed withwait-for2@background_tasks.await_on_shutdownannotation to mark coroutines as "not to cancel on shutdown" (and thereby also fixes app reset fails due to improper I/O #4312)clean up multiprocessing to not get "leaked semaphore" warnings (fix was described in Leaked Semaphore Objects #4131 (comment))Also this PR makes some minor improvements:
background_tasksdocumentation