https://github.com/python/cpython/commit/fa58e75a8605146a89ef72b58b4529669ac48366 commit: fa58e75a8605146a89ef72b58b4529669ac48366 branch: main author: Guido van Rossum <[email protected]> committer: gvanrossum <[email protected]> date: 2024-04-09T08:17:28-07:00 summary:
gh-116720: Fix corner cases of taskgroups (#117407) This prevents external cancellations of a task group's parent task to be dropped when an internal cancellation happens at the same time. Also strengthen the semantics of uncancel() to clear self._must_cancel when the cancellation count reaches zero. Co-Authored-By: Tin Tvrtković <[email protected]> Co-Authored-By: Arthur Tacca files: A Misc/NEWS.d/next/Library/2024-04-04-15-28-12.gh-issue-116720.aGhXns.rst M Doc/library/asyncio-task.rst M Doc/whatsnew/3.13.rst M Lib/asyncio/taskgroups.py M Lib/asyncio/tasks.py M Lib/test/test_asyncio/test_taskgroups.py M Lib/test/test_asyncio/test_tasks.py M Modules/_asynciomodule.c diff --git a/Doc/library/asyncio-task.rst b/Doc/library/asyncio-task.rst index 3b10a0d628a86e..3d300c37419f13 100644 --- a/Doc/library/asyncio-task.rst +++ b/Doc/library/asyncio-task.rst @@ -392,6 +392,27 @@ is also included in the exception group. The same special case is made for :exc:`KeyboardInterrupt` and :exc:`SystemExit` as in the previous paragraph. +Task groups are careful not to mix up the internal cancellation used to +"wake up" their :meth:`~object.__aexit__` with cancellation requests +for the task in which they are running made by other parties. +In particular, when one task group is syntactically nested in another, +and both experience an exception in one of their child tasks simultaneously, +the inner task group will process its exceptions, and then the outer task group +will receive another cancellation and process its own exceptions. + +In the case where a task group is cancelled externally and also must +raise an :exc:`ExceptionGroup`, it will call the parent task's +:meth:`~asyncio.Task.cancel` method. This ensures that a +:exc:`asyncio.CancelledError` will be raised at the next +:keyword:`await`, so the cancellation is not lost. + +Task groups preserve the cancellation count +reported by :meth:`asyncio.Task.cancelling`. + +.. versionchanged:: 3.13 + + Improved handling of simultaneous internal and external cancellations + and correct preservation of cancellation counts. Sleeping ======== @@ -1369,6 +1390,15 @@ Task Object catching :exc:`CancelledError`, it needs to call this method to remove the cancellation state. + When this method decrements the cancellation count to zero, + the method checks if a previous :meth:`cancel` call had arranged + for :exc:`CancelledError` to be thrown into the task. + If it hasn't been thrown yet, that arrangement will be + rescinded (by resetting the internal ``_must_cancel`` flag). + + .. versionchanged:: 3.13 + Changed to rescind pending cancellation requests upon reaching zero. + .. method:: cancelling() Return the number of pending cancellation requests to this Task, i.e., diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 707dcaa160d653..d971beb27bbf27 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -196,13 +196,6 @@ Other Language Changes (Contributed by Sebastian Pipping in :gh:`115623`.) -* When :func:`asyncio.TaskGroup.create_task` is called on an inactive - :class:`asyncio.TaskGroup`, the given coroutine will be closed (which - prevents a :exc:`RuntimeWarning` about the given coroutine being - never awaited). - - (Contributed by Arthur Tacca and Jason Zhang in :gh:`115957`.) - * The :func:`ssl.create_default_context` API now includes :data:`ssl.VERIFY_X509_PARTIAL_CHAIN` and :data:`ssl.VERIFY_X509_STRICT` in its default flags. @@ -300,6 +293,33 @@ asyncio with the tasks being completed. (Contributed by Justin Arthur in :gh:`77714`.) +* When :func:`asyncio.TaskGroup.create_task` is called on an inactive + :class:`asyncio.TaskGroup`, the given coroutine will be closed (which + prevents a :exc:`RuntimeWarning` about the given coroutine being + never awaited). + (Contributed by Arthur Tacca and Jason Zhang in :gh:`115957`.) + +* Improved behavior of :class:`asyncio.TaskGroup` when an external cancellation + collides with an internal cancellation. For example, when two task groups + are nested and both experience an exception in a child task simultaneously, + it was possible that the outer task group would hang, because its internal + cancellation was swallowed by the inner task group. + + In the case where a task group is cancelled externally and also must + raise an :exc:`ExceptionGroup`, it will now call the parent task's + :meth:`~asyncio.Task.cancel` method. This ensures that a + :exc:`asyncio.CancelledError` will be raised at the next + :keyword:`await`, so the cancellation is not lost. + + An added benefit of these changes is that task groups now preserve the + cancellation count (:meth:`asyncio.Task.cancelling`). + + In order to handle some corner cases, :meth:`asyncio.Task.uncancel` may now + reset the undocumented ``_must_cancel`` flag when the cancellation count + reaches zero. + + (Inspired by an issue reported by Arthur Tacca in :gh:`116720`.) + * Add :meth:`asyncio.Queue.shutdown` (along with :exc:`asyncio.QueueShutDown`) for queue termination. (Contributed by Laurie Opperman and Yves Duprat in :gh:`104228`.) diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 57f01230159319..f2ee9648c43876 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -77,12 +77,6 @@ async def __aexit__(self, et, exc, tb): propagate_cancellation_error = exc else: propagate_cancellation_error = None - if self._parent_cancel_requested: - # If this flag is set we *must* call uncancel(). - if self._parent_task.uncancel() == 0: - # If there are no pending cancellations left, - # don't propagate CancelledError. - propagate_cancellation_error = None if et is not None: if not self._aborting: @@ -130,6 +124,13 @@ async def __aexit__(self, et, exc, tb): if self._base_error is not None: raise self._base_error + if self._parent_cancel_requested: + # If this flag is set we *must* call uncancel(). + if self._parent_task.uncancel() == 0: + # If there are no pending cancellations left, + # don't propagate CancelledError. + propagate_cancellation_error = None + # Propagate CancelledError if there is one, except if there # are other errors -- those have priority. if propagate_cancellation_error is not None and not self._errors: @@ -139,6 +140,12 @@ async def __aexit__(self, et, exc, tb): self._errors.append(exc) if self._errors: + # If the parent task is being cancelled from the outside + # of the taskgroup, un-cancel and re-cancel the parent task, + # which will keep the cancel count stable. + if self._parent_task.cancelling(): + self._parent_task.uncancel() + self._parent_task.cancel() # Exceptions are heavy objects that can have object # cycles (bad for GC); let's not keep a reference to # a bunch of them. diff --git a/Lib/asyncio/tasks.py b/Lib/asyncio/tasks.py index 7fb697b9441c33..dadcb5b5f36bd7 100644 --- a/Lib/asyncio/tasks.py +++ b/Lib/asyncio/tasks.py @@ -255,6 +255,8 @@ def uncancel(self): """ if self._num_cancels_requested > 0: self._num_cancels_requested -= 1 + if self._num_cancels_requested == 0: + self._must_cancel = False return self._num_cancels_requested def __eager_start(self): diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 1ec8116953f811..4852536defc93d 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -833,6 +833,72 @@ async def run_coro_after_tg_closes(): loop = asyncio.get_event_loop() loop.run_until_complete(run_coro_after_tg_closes()) + async def test_cancelling_level_preserved(self): + async def raise_after(t, e): + await asyncio.sleep(t) + raise e() + + try: + async with asyncio.TaskGroup() as tg: + tg.create_task(raise_after(0.0, RuntimeError)) + except* RuntimeError: + pass + self.assertEqual(asyncio.current_task().cancelling(), 0) + + async def test_nested_groups_both_cancelled(self): + async def raise_after(t, e): + await asyncio.sleep(t) + raise e() + + try: + async with asyncio.TaskGroup() as outer_tg: + try: + async with asyncio.TaskGroup() as inner_tg: + inner_tg.create_task(raise_after(0, RuntimeError)) + outer_tg.create_task(raise_after(0, ValueError)) + except* RuntimeError: + pass + else: + self.fail("RuntimeError not raised") + self.assertEqual(asyncio.current_task().cancelling(), 1) + except* ValueError: + pass + else: + self.fail("ValueError not raised") + self.assertEqual(asyncio.current_task().cancelling(), 0) + + async def test_error_and_cancel(self): + event = asyncio.Event() + + async def raise_error(): + event.set() + await asyncio.sleep(0) + raise RuntimeError() + + async def inner(): + try: + async with taskgroups.TaskGroup() as tg: + tg.create_task(raise_error()) + await asyncio.sleep(1) + self.fail("Sleep in group should have been cancelled") + except* RuntimeError: + self.assertEqual(asyncio.current_task().cancelling(), 1) + self.assertEqual(asyncio.current_task().cancelling(), 1) + await asyncio.sleep(1) + self.fail("Sleep after group should have been cancelled") + + async def outer(): + t = asyncio.create_task(inner()) + await event.wait() + self.assertEqual(t.cancelling(), 0) + t.cancel() + self.assertEqual(t.cancelling(), 1) + with self.assertRaises(asyncio.CancelledError): + await t + self.assertTrue(t.cancelled()) + + await outer() + if __name__ == "__main__": unittest.main() diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index bc6d88e65a4966..5b09c81faef62a 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -684,6 +684,30 @@ def on_timeout(): finally: loop.close() + def test_uncancel_resets_must_cancel(self): + + async def coro(): + await fut + return 42 + + loop = asyncio.new_event_loop() + fut = asyncio.Future(loop=loop) + task = self.new_task(loop, coro()) + loop.run_until_complete(asyncio.sleep(0)) # Get task waiting for fut + fut.set_result(None) # Make task runnable + try: + task.cancel() # Enter cancelled state + self.assertEqual(task.cancelling(), 1) + self.assertTrue(task._must_cancel) + + task.uncancel() # Undo cancellation + self.assertEqual(task.cancelling(), 0) + self.assertFalse(task._must_cancel) + finally: + res = loop.run_until_complete(task) + self.assertEqual(res, 42) + loop.close() + def test_cancel(self): def gen(): diff --git a/Misc/NEWS.d/next/Library/2024-04-04-15-28-12.gh-issue-116720.aGhXns.rst b/Misc/NEWS.d/next/Library/2024-04-04-15-28-12.gh-issue-116720.aGhXns.rst new file mode 100644 index 00000000000000..39c7d6b8a1e978 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-04-04-15-28-12.gh-issue-116720.aGhXns.rst @@ -0,0 +1,18 @@ +Improved behavior of :class:`asyncio.TaskGroup` when an external cancellation +collides with an internal cancellation. For example, when two task groups +are nested and both experience an exception in a child task simultaneously, +it was possible that the outer task group would misbehave, because +its internal cancellation was swallowed by the inner task group. + +In the case where a task group is cancelled externally and also must +raise an :exc:`ExceptionGroup`, it will now call the parent task's +:meth:`~asyncio.Task.cancel` method. This ensures that a +:exc:`asyncio.CancelledError` will be raised at the next +:keyword:`await`, so the cancellation is not lost. + +An added benefit of these changes is that task groups now preserve the +cancellation count (:meth:`asyncio.Task.cancelling`). + +In order to handle some corner cases, :meth:`asyncio.Task.uncancel` may now +reset the undocumented ``_must_cancel`` flag when the cancellation count +reaches zero. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 29246cfa6afd00..b886051186de9c 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2393,6 +2393,9 @@ _asyncio_Task_uncancel_impl(TaskObj *self) { if (self->task_num_cancels_requested > 0) { self->task_num_cancels_requested -= 1; + if (self->task_num_cancels_requested == 0) { + self->task_must_cancel = 0; + } } return PyLong_FromLong(self->task_num_cancels_requested); } _______________________________________________ Python-checkins mailing list -- [email protected] To unsubscribe send an email to [email protected] https://mail.python.org/mailman3/lists/python-checkins.python.org/ Member address: [email protected]
