Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

gh-101581 Optionally prevent child tasks from being cancelled in asyncio taskgroups #101648

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
Loading
from

Conversation

DontPanicO
Copy link

@DontPanicO DontPanicO commented Feb 7, 2023

`asyncio.TaskGroup` cancels all child tasks when one raises. Setting `defer_errors=True` will prevent this
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Feb 7, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

Add explanation about `defer_errors=True` bheaviour in `asyncio.TaskGroup` docstring
@arhadthedev arhadthedev added topic-asyncio stdlib Python modules in the Lib dir labels Apr 1, 2023
@arhadthedev
Copy link
Member

@gvanrossum @kumaraditya303 (as participants of the parent issue)

@1st1, @asvetlov, @graingert (as asyncio experts)

@@ -198,7 +201,8 @@ def _on_task_done(self, task):
return

self._errors.append(exc)
if self._is_base_error(exc) and self._base_error is None:
is_base_error = self._is_base_error(exc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @DontPanicO, I'm trying to understand why is_base_error is needed. Please help me understand why that is preferable to self._is_base_error(exc). Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Hi @willingc . I stored self._is_base_error(exc) result in a variable since it's used twice: once on line 205 and once on line 238

Copy link
Contributor

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

As I've left a reply in the discourse thread, I'm concerned about potential memory leaks when a taskgroup with defer_errors=True is long-lived (e.g., throughout the entire lifecycle of the event loop, or the "server" usage scenario) because the current implementation stores all unhandled exceptions in self._errors for an indefinite length of time.

I'd like to have some discussion on the design of handling the exceptions for long-lived cases.

Ideas:

  1. Rename defer_errors=True to ignore_errors=True and let it drop all unhandled errors immediately to keep running.
    • Cons: yet another silent swallowing of exceptions...
  2. Replace defer_errors=True with delegate_errors=handler_func, where handler_func shares the compatible interface with loop.call_exception_handler(). If it is None (default), behave like the original taskgroup.
    • Cons: what whould be the difference to task.add_done_callback() by the caller?
  3. Split the behavior of "store-and-raising up exceptions as exception group" and "skipping cancellation of child tasks" as two separate options:
    • asyncio.TaskGroup(raise_errors: bool, cancel_together: bool)
      or asyncio.TaskGroup(raise_errors: bool, run_to_completion: bool)
      • raise_errors=True: stores unhandled exceptions and raises up as exception group upon exit, otherwise silently drop them.
      • cancel_together=False / run_to_completion=True: does not cancel other children when it sees the first unhandled exception.
    • Cons: too complicated

The current aiotools.Supervisor works like the first one. I'm using add_done_callback() to catch unhandled exceptions to implement safe_as_completed() and alikes.

This is why I wanted to have a separate API to avoid confusion because the semantic is different at the first place (even though it may be technically feasible to implement as an option to TaskGroup):

  • TaskGroup: all child tasks share the same destiny (always cancelled together, exception group)
    • May have strong references to the child tasks
    • Intended to be short-lived
  • Supervisor: each child task has its own destiny (run to completion, individual exception handling)
    • Should have weak references to the child tasks
    • Intended to be long-lived
    • Child tasks are cancelled together only when explicitly requested via shutdown()

In fact, Supervisor (or TaskScope) is more versatile: it can be used as an underlying lower-level primitive to write higher-level coroutine utilities—that's why I've suggested a naming like "TaskScope". Even TaskGroup could be written from TaskScope with custom task callbacks or a custom exception handler that immediately calls shutdown() upon the first exception and store-and-raise up exceptions as an exception group. #105011 demonstrates this.

@1st1
Copy link
Member

1st1 commented Sep 12, 2024

+1 to the @achimnol's idea of TaskScope. We do need to figure out some details but his PR is pretty close.

@1st1 1st1 added the sprint label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.