From 9f96cea0deb91dcdeb79d143fb89c0280e6e4add Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Sat, 18 Mar 2023 09:46:27 +0000 Subject: [PATCH 01/11] document use of uncancel() when suppressing CancelledError --- Doc/library/asyncio-task.rst | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Doc/library/asyncio-task.rst b/Doc/library/asyncio-task.rst index a0900cd25a7731..ac68dc8ef36c1a 100644 --- a/Doc/library/asyncio-task.rst +++ b/Doc/library/asyncio-task.rst @@ -306,7 +306,10 @@ The asyncio components that enable structured concurrency, like :class:`asyncio.TaskGroup` and :func:`asyncio.timeout`, are implemented using cancellation internally and might misbehave if a coroutine swallows :exc:`asyncio.CancelledError`. Similarly, user code -should not call :meth:`uncancel `. +should not generally call :meth:`uncancel `. +However, in such cases when supprssing :exc:`asynci.CancelledError` is +truly desired, it is necessary to also call :meth:`uncancel` to completely +remove the cancellation state. .. _taskgroups: @@ -1148,7 +1151,9 @@ Task Object Therefore, unlike :meth:`Future.cancel`, :meth:`Task.cancel` does not guarantee that the Task will be cancelled, although suppressing cancellation completely is not common and is actively - discouraged. + discouraged. Should the coroutine nevertheless decide to suppress + the cancellation, it needs to call :meth:`Task.uncancel` in addition + to catching the exception. .. versionchanged:: 3.9 Added the *msg* parameter. @@ -1238,6 +1243,10 @@ Task Object with :meth:`uncancel`. :class:`TaskGroup` context managers use :func:`uncancel` in a similar fashion. + If end-user code is, for some reason, suppresing cancellation by + catching :exc:`CancelledError`, it needs to call this method to remove + the cancellation state. + .. method:: cancelling() Return the number of pending cancellation requests to this Task, i.e., From 07517b7703ff4a692a48c9a9cf46c76c9b6df4a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Sat, 18 Mar 2023 10:21:44 +0000 Subject: [PATCH 02/11] Create unittest --- Lib/test/test_asyncio/test_timeouts.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Lib/test/test_asyncio/test_timeouts.py b/Lib/test/test_asyncio/test_timeouts.py index b9bac6f783776b..d6c8a88e290044 100644 --- a/Lib/test/test_asyncio/test_timeouts.py +++ b/Lib/test/test_asyncio/test_timeouts.py @@ -247,6 +247,29 @@ async def test_nested_timeout_in_finally(self): async with asyncio.timeout(0.01): await asyncio.sleep(10) + async def test_timeout_after_cancellation(self): + try: + asyncio.current_task().cancel() + await asyncio.sleep(1) # work which will be cancelled + except asyncio.CancelledError: + pass + finally: + with self.assertRaises(TimeoutError): + async with asyncio.timeout(0.0): + await asyncio.sleep(1) # some cleanup + + async def test_cancel_in_timeout_after_cancellation(self): + try: + asyncio.current_task().cancel() + await asyncio.sleep(1) # work which will be cancelled + except asyncio.CancelledError: + pass + finally: + with self.assertRaises(asyncio.CancelledError): + async with asyncio.timeout(1.0): + asyncio.current_task().cancel() + await asyncio.sleep(2) # some cleanup + if __name__ == '__main__': unittest.main() From 1150db0046db5bb0e4742c917a37872492152327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Sat, 18 Mar 2023 13:47:37 +0000 Subject: [PATCH 03/11] record previous cancel state in timeout context manager --- Lib/asyncio/timeouts.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/asyncio/timeouts.py b/Lib/asyncio/timeouts.py index 94d25535fbc059..ff087fa7a83c35 100644 --- a/Lib/asyncio/timeouts.py +++ b/Lib/asyncio/timeouts.py @@ -72,6 +72,7 @@ def __repr__(self) -> str: async def __aenter__(self) -> "Timeout": self._state = _State.ENTERED self._task = tasks.current_task() + self._cancelling = self._task.cancelling() if self._task is None: raise RuntimeError("Timeout should be used inside a task") self.reschedule(self._when) @@ -92,8 +93,8 @@ async def __aexit__( if self._state is _State.EXPIRING: self._state = _State.EXPIRED - if self._task.uncancel() == 0 and exc_type is exceptions.CancelledError: - # Since there are no outstanding cancel requests, we're + if self._task.uncancel() <= self._cancelling and exc_type is exceptions.CancelledError: + # Since there are no new cancel requests, we're # handling this. raise TimeoutError elif self._state is _State.ENTERED: From d207fb1851c77b728d32c7eeb0d0d3751759d3c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Sat, 18 Mar 2023 13:48:46 +0000 Subject: [PATCH 04/11] explicitly raise from cancelled error --- Lib/asyncio/timeouts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/asyncio/timeouts.py b/Lib/asyncio/timeouts.py index ff087fa7a83c35..3aa1a609f4e077 100644 --- a/Lib/asyncio/timeouts.py +++ b/Lib/asyncio/timeouts.py @@ -96,7 +96,7 @@ async def __aexit__( if self._task.uncancel() <= self._cancelling and exc_type is exceptions.CancelledError: # Since there are no new cancel requests, we're # handling this. - raise TimeoutError + raise TimeoutError from exc_val elif self._state is _State.ENTERED: self._state = _State.EXITED From 912f26d54614bf5b0385d87dd6cada11e5f3d9c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Mon, 20 Mar 2023 11:10:47 +0000 Subject: [PATCH 05/11] Update Doc/library/asyncio-task.rst Co-authored-by: Guido van Rossum --- Doc/library/asyncio-task.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/library/asyncio-task.rst b/Doc/library/asyncio-task.rst index ac68dc8ef36c1a..8bdea5cfb4d2d0 100644 --- a/Doc/library/asyncio-task.rst +++ b/Doc/library/asyncio-task.rst @@ -307,7 +307,7 @@ The asyncio components that enable structured concurrency, like are implemented using cancellation internally and might misbehave if a coroutine swallows :exc:`asyncio.CancelledError`. Similarly, user code should not generally call :meth:`uncancel `. -However, in such cases when supprssing :exc:`asynci.CancelledError` is +However, in cases when suppressing :exc:`asyncio.CancelledError` is truly desired, it is necessary to also call :meth:`uncancel` to completely remove the cancellation state. From ee574fc7790373e30874f2955256d48fb6d48ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Mon, 20 Mar 2023 11:35:05 +0000 Subject: [PATCH 06/11] Clarify how code should "ignore" CancelledError --- Doc/library/asyncio-task.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Doc/library/asyncio-task.rst b/Doc/library/asyncio-task.rst index 8bdea5cfb4d2d0..926391ff19805f 100644 --- a/Doc/library/asyncio-task.rst +++ b/Doc/library/asyncio-task.rst @@ -300,7 +300,8 @@ in the task at the next opportunity. It is recommended that coroutines use ``try/finally`` blocks to robustly perform clean-up logic. In case :exc:`asyncio.CancelledError` is explicitly caught, it should generally be propagated when -clean-up is complete. Most code can safely ignore :exc:`asyncio.CancelledError`. +clean-up is complete. :exc:`asyncio.CancelledError` is a subclass of +:exc:`BaseException` so most code will not need to be aware of it. The asyncio components that enable structured concurrency, like :class:`asyncio.TaskGroup` and :func:`asyncio.timeout`, From c91dae79b90081e92dea5931394da6555bcabbcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Mon, 20 Mar 2023 14:01:30 +0000 Subject: [PATCH 07/11] Apply suggestions from code review Co-authored-by: Alex Waygood --- Doc/library/asyncio-task.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/asyncio-task.rst b/Doc/library/asyncio-task.rst index 926391ff19805f..d908e45b3c8d8b 100644 --- a/Doc/library/asyncio-task.rst +++ b/Doc/library/asyncio-task.rst @@ -300,7 +300,7 @@ in the task at the next opportunity. It is recommended that coroutines use ``try/finally`` blocks to robustly perform clean-up logic. In case :exc:`asyncio.CancelledError` is explicitly caught, it should generally be propagated when -clean-up is complete. :exc:`asyncio.CancelledError` is a subclass of +clean-up is complete. :exc:`asyncio.CancelledError` directly subclasses :exc:`BaseException` so most code will not need to be aware of it. The asyncio components that enable structured concurrency, like @@ -309,7 +309,7 @@ are implemented using cancellation internally and might misbehave if a coroutine swallows :exc:`asyncio.CancelledError`. Similarly, user code should not generally call :meth:`uncancel `. However, in cases when suppressing :exc:`asyncio.CancelledError` is -truly desired, it is necessary to also call :meth:`uncancel` to completely +truly desired, it is necessary to also call ``uncancel()`` to completely remove the cancellation state. .. _taskgroups: From 902691e04f83b04fb559a7b4f2a0ae364032b8eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Mon, 20 Mar 2023 15:59:13 +0000 Subject: [PATCH 08/11] Add unittest for asyncio.TimeoutError.__cause__ --- Lib/test/test_asyncio/test_timeouts.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Lib/test/test_asyncio/test_timeouts.py b/Lib/test/test_asyncio/test_timeouts.py index d6c8a88e290044..8b6b9a1fea0be8 100644 --- a/Lib/test/test_asyncio/test_timeouts.py +++ b/Lib/test/test_asyncio/test_timeouts.py @@ -270,6 +270,13 @@ async def test_cancel_in_timeout_after_cancellation(self): asyncio.current_task().cancel() await asyncio.sleep(2) # some cleanup + async def test_timeout_exception_cause (self): + with self.assertRaises(asyncio.TimeoutError) as exc: + async with asyncio.timeout(0): + await asyncio.sleep(1) + cause = exc.exception.__cause__ + assert isinstance(cause, asyncio.CancelledError) + if __name__ == '__main__': unittest.main() From 07c40ccaca3355de04c58e187afb79dfbb7f1bb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Wed, 22 Mar 2023 16:15:31 +0000 Subject: [PATCH 09/11] add news --- .../next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst diff --git a/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst b/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst new file mode 100644 index 00000000000000..423a9aadc97c40 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst @@ -0,0 +1,3 @@ +The `asyncio.timeout()` now works reliably even when performing cleanup due +to task cancellation. Previously the context manager could raise a +`asyncio.CancelledError` instead of an `asyncio.TimeoutError` in such cases. From a6f3114509421387e6388640b391e4e21dc40396 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Wed, 22 Mar 2023 16:30:37 +0000 Subject: [PATCH 10/11] Rename news file --- ....NEcljy.rst => 2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Misc/NEWS.d/next/Library/{2023-03-22-16-15-18.bpo-102780.NEcljy.rst => 2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst} (100%) diff --git a/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst b/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst similarity index 100% rename from Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst rename to Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst From 6e299fa022dbcdb075e362d398872d525341994f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristj=C3=A1n=20Valur=20J=C3=B3nsson?= Date: Wed, 22 Mar 2023 16:40:47 +0000 Subject: [PATCH 11/11] fix news entry --- .../Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst b/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst index 423a9aadc97c40..2aaffe34b86414 100644 --- a/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst +++ b/Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst @@ -1,3 +1,3 @@ -The `asyncio.timeout()` now works reliably even when performing cleanup due -to task cancellation. Previously the context manager could raise a -`asyncio.CancelledError` instead of an `asyncio.TimeoutError` in such cases. +The :class:`asyncio.Timeout` context manager now works reliably even when performing cleanup due +to task cancellation. Previously it could raise a +:exc:`~asyncio.CancelledError` instead of an :exc:`~asyncio.TimeoutError` in such cases.