Skip to content

use background_tasks.create over bare asyncio.create_task #4551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 31, 2025

Conversation

evnchn
Copy link
Collaborator

@evnchn evnchn commented Mar 30, 2025

Should fix #4500

TL-DR: As in title. Should avoid GC messing up asyncio tasks since hard reference is kept.


Read specifically #4500 (comment) for the affected line in NiceGUI 2.13.0

Apparently, from articles in #4500 (comment), since asyncio does not itself keep a hard reference to the task created by asyncio.create_task, it can easily by GC-ed and cause Task was destroyed but it is pending!

Sitting in front of us (literally, it's 4 lines before the change in #4466), we just used background_tasks.create, which keeps a hard reference by putting all tasks into running_tasks. It has never caused trouble, or at least if it would, it should have surfaced a long long time ago (too lazy to git blame, but I bet this function introduced several versions before NiceGUI 2.13.0)

def create(coroutine: Awaitable, *, name: str = 'unnamed task') -> asyncio.Task:
"""Wraps a loop.create_task call and ensures there is an exception handler added to the task.
If the task raises an exception, it is logged and handled by the global exception handlers.
Also a reference to the task is kept until it is done, so that the task is not garbage collected mid-execution.
See https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task.
"""
assert core.loop is not None
coroutine = coroutine if asyncio.iscoroutine(coroutine) else asyncio.wait_for(coroutine, None)
task: asyncio.Task = core.loop.create_task(coroutine, name=name)
task.add_done_callback(_handle_task_result)
running_tasks.add(task)
task.add_done_callback(running_tasks.discard)
return task

@falkoschindler falkoschindler added bug Type/scope: A problem that needs fixing review Status: PR is open and needs review 🟠 major Priority: Important, but not urgent 🌿 intermediate Difficulty: Requires moderate understanding labels Mar 31, 2025
@falkoschindler falkoschindler self-requested a review March 31, 2025 05:30
@falkoschindler falkoschindler added this to the 2.14 milestone Mar 31, 2025
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! After @chriswi93 successfully tested this PR, I think it's ready to merge. Thanks for the contribution, @evnchn! And thanks for your help, @chriswi93!

@falkoschindler falkoschindler merged commit 479c977 into zauberzeug:main Mar 31, 2025
1 check passed
@evnchn evnchn deleted the avoid-task-without-ref branch March 31, 2025 21:03
@JudeMcbeath

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type/scope: A problem that needs fixing 🌿 intermediate Difficulty: Requires moderate understanding 🟠 major Priority: Important, but not urgent review Status: PR is open and needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task was destroyed but it is pending!
3 participants