python / cpython

The Python programming language
https://www.python.org
Other
63.44k stars 30.38k forks source link

Optionally prevent child tasks from being cancelled in `asyncio.TaskGroup` #101581

Open DontPanicO opened 1 year ago

DontPanicO commented 1 year ago

Feature or enhancement

Add a flag to asyncio.TaskGroup to control whether, if a child task crashes, other should be cancelled or not.

Pitch

Currently asyncio.TaskGroup always cancels all child tasks if one fails. Even if that's the most common use case, I'd like to be able to switch from the current behaviour to prevent child tasks cancellation when one failure occurs and raise an ExceptionGroup with all exceptions raised (only after other tasks completed).

Example usage:

async def zero_div():
    1 / 0

async def wait_some(d):
    await asyncio.sleep(d)
    print(f'finished after {d}')

async def main():
    async with asyncio.TaskGroup(abort_on_first_exception=False) as tg:
        tg.create_task(wait_some(2))
        tg.create_task(wait_some(3))
        tg.create_task(zero_div())

asyncio.run(main())
# Should print out:
# finished after 2
# finished after 3
# and then raise an `ExceptionGroup` with a `ZeroDivisionError`

Looking at asyncio.TaskGroup source code, it seems that it could be achieved by adding the abort_on_first_exception (or whatever it should be named) flag to the __init__ method and then modify _on_task_done as follow:

class TaskGroup:

    def __init__(self, abort_on_first_exception=True):
        ...
        self._abort_on_first_exception = abort_on_first_exception

    ...

    def _on_task_done(self, task):
        ...
        self._errors.append(exc)
        is_base_error = self._is_base_error(exc)
        if is_base_error and self._base_error is None:
            self._base_error = exc

        if self._parent_task.done():
            # Not sure if this case is possible, but we want to handle
            # it anyways.
            self._loop.call_exception_handler({
                'message': f'Task {task!r} has errored out but its parent '
                           f'task {self._parent_task} is already completed',
                'exception': exc,
                'task': task,
            })
            return

        if not self._aborting and not self._parent_cancel_requested:
            #comment skipped for brevity
            if is_base_error or self._abort_on_first_exception:
                self._abort()
                self._parent_cancel_requested = True
                self._parent_task.cancel()

If it's reasonable to have it in asyncio, I'll be glad to submit a PR for it.

Previous discussion

Post on discuss

Linked PRs

gvanrossum commented 1 year ago

Yeah, I think this is reasonable. We need to bikeshed on the name of the flag, and I would like to ensure that when the awaiting task (i.e., the TaskGroup itself) is cancelled, all awaited tasks (i.e., all running tasks that were created by the TaskGroup) are cancelled. There are tests for this, look for r.cancel() in test_taskgroups.py, we should probably clone some of these and run them with the new flag as well.

DontPanicO commented 1 year ago

We need to bikeshed on the name of the flag

Yes, it's quite hard to figure out a name that's descriptive enough but reasonably short. May abort_on_child_exception ?

I would like to ensure that when the awaiting task (i.e., the TaskGroup itself) is cancelled, all awaited tasks (i.e., all running tasks that were created by the TaskGroup) are cancelled. There are tests for this, look for r.cancel() in test_taskgroups.py, we should probably clone some of these and run them with the new flag as well.

In the local clone of my fork I've already defined new tests for the new intended behaviour and I've copied the ones you've pointed out (setting the new flag to prevent child tasks cancellation).

Should I open a PR now or it'd be better to wait for the flag name to have been found?

gvanrossum commented 1 year ago

abort_on_child_exception still feels too long. Maybe shield_errors=True (default False)? Or propagate_errors=False (default True)? Or spread_errors=True (default False)? (You see a pattern emerge, I'd like a verb and a noun, and the noun should probably be "errors". :-)

DontPanicO commented 1 year ago

I consider shield_errors (default False) to be the closest. propagate_errors may be misleading since exceptions still propagate after other child tasks have been completed. I'd like to follow this one, if there are no objections to shield_errors or better proposals.

arhadthedev commented 1 year ago

Maybe postpone_errors (default False)?

gvanrossum commented 1 year ago

Oh! defer_errors=True (default False).

DontPanicO commented 1 year ago

Oh! defer_errors=True (default False).

Fine with me. We all agree on defer_errors=True (default False)?

kumaraditya303 commented 1 year ago

Oh! defer_errors=True (default False).

SGTM

gvanrossum commented 1 year ago

So, since there is still more discussion on Discourse, I'd like to get a review from @achimnol, who (I hope) is collecting all the various possible solutions, e.g. Supervisor (also yours) and PersistentTaskGroup; there's a somewhat recent (post-US-PyCon) writeup in Discourse: https://discuss.python.org/t/asyncio-tasks-and-exception-handling-recommended-idioms/23806/9