From 9f77facb562b8ff15516f32fc431d4c12a7b88df Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 11:18:43 -0500 Subject: [PATCH 1/9] Fix TimerContext not uncancelling the current task effectively the same fix as https://github.com/aio-libs/async-timeout/pull/363 --- aiohttp/helpers.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index bd8c11a3406..abd37c64edf 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -692,6 +692,17 @@ def __exit__( return +if sys.version_info >= (3, 11): + + def _uncancel_task(task: "asyncio.Task[object]") -> None: + task.uncancel() + +else: + + def _uncancel_task(task: "asyncio.Task[object]") -> None: + pass + + class TimerContext(BaseTimerContext): """Low resolution timeout context manager""" @@ -723,10 +734,16 @@ def __exit__( exc_val: Optional[BaseException], exc_tb: Optional[TracebackType], ) -> Optional[bool]: + enter_task: Optional[asyncio.Task[Any]] = None if self._tasks: - self._tasks.pop() # type: ignore[unused-awaitable] + enter_task = self._tasks.pop() if exc_type is asyncio.CancelledError and self._cancelled: + assert enter_task is not None + # The timeout was hit, and the task was cancelled + # so we need to uncancel the last task that entered the context manager + # since the cancellation should not leak out of the context manager + _uncancel_task(enter_task) raise asyncio.TimeoutError from None return None From 48a9f57b9b0c37e3f5c6220488f5f00dd976c729 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 12:13:24 -0500 Subject: [PATCH 2/9] inline --- aiohttp/helpers.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index abd37c64edf..c536f771ee8 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -692,17 +692,6 @@ def __exit__( return -if sys.version_info >= (3, 11): - - def _uncancel_task(task: "asyncio.Task[object]") -> None: - task.uncancel() - -else: - - def _uncancel_task(task: "asyncio.Task[object]") -> None: - pass - - class TimerContext(BaseTimerContext): """Low resolution timeout context manager""" @@ -743,7 +732,8 @@ def __exit__( # The timeout was hit, and the task was cancelled # so we need to uncancel the last task that entered the context manager # since the cancellation should not leak out of the context manager - _uncancel_task(enter_task) + if sys.version_info >= (3, 11): + enter_task.uncancel() raise asyncio.TimeoutError from None return None From 1e2672864972327f7faa44762d117a6723d26608 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 12:21:03 -0500 Subject: [PATCH 3/9] coverage --- tests/test_helpers.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index e79e168e753..60b25ec641f 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -425,6 +425,24 @@ def test_timer_context_not_cancelled() -> None: assert not m_asyncio.current_task.return_value.cancel.called +@pytest.mark.skipif( + sys.version_info < (3, 11), reason="Python 3.11+ is required for .cancelling()" +) +async def test_timer_context_timeout_does_not_leak_upward() -> None: + """Verify that the TimerContext does not leak cancellation outside the context manager.""" + loop = asyncio.get_running_loop() + ctx = helpers.TimerContext(loop) + current_task = asyncio.current_task() + with pytest.raises(asyncio.TimeoutError): + with ctx: + assert current_task.cancelling() == 0 + loop.call_soon(ctx.timeout) + await asyncio.sleep(1) + + # After the context manager exits, the task should no longer be cancelling + assert current_task.cancelling() == 0 + + def test_timer_context_no_task(loop: asyncio.AbstractEventLoop) -> None: with pytest.raises(RuntimeError): with helpers.TimerContext(loop): From 67440cdbd2dc1ae67495d06cc1acbcfb7338a377 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 12:23:26 -0500 Subject: [PATCH 4/9] changelog --- CHANGES/9326.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/9326.bugfix.rst diff --git a/CHANGES/9326.bugfix.rst b/CHANGES/9326.bugfix.rst new file mode 100644 index 00000000000..4689941708f --- /dev/null +++ b/CHANGES/9326.bugfix.rst @@ -0,0 +1 @@ +Fixed cancellation leaking upwards on timeout -- by :user:`bdraco`. From 0a3542ba0a0c611de4fb60412a9758cdd3c0e312 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 12:27:04 -0500 Subject: [PATCH 5/9] assert --- tests/test_helpers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 60b25ec641f..8e9bb29482d 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -433,6 +433,7 @@ async def test_timer_context_timeout_does_not_leak_upward() -> None: loop = asyncio.get_running_loop() ctx = helpers.TimerContext(loop) current_task = asyncio.current_task() + assert current_task is not None with pytest.raises(asyncio.TimeoutError): with ctx: assert current_task.cancelling() == 0 From 40de68d58bc840400023fffce6b749ed2ff70d3a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 12:58:56 -0500 Subject: [PATCH 6/9] fix cancellation being swallowed as well --- aiohttp/helpers.py | 18 ++++++++++++++---- tests/test_helpers.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index c536f771ee8..9a23ecc7eab 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -699,6 +699,7 @@ def __init__(self, loop: asyncio.AbstractEventLoop) -> None: self._loop = loop self._tasks: List[asyncio.Task[Any]] = [] self._cancelled = False + self._cancelling = 0 def assert_timeout(self) -> None: """Raise TimeoutError if timer has already been cancelled.""" @@ -707,10 +708,15 @@ def assert_timeout(self) -> None: def __enter__(self) -> BaseTimerContext: task = asyncio.current_task(loop=self._loop) - if task is None: raise RuntimeError("Timeout context manager should be used inside a task") + if sys.version_info >= (3, 11): + # Remember if the task was already cancelling + # so when we __exit__ we can decide if we should + # raise asyncio.TimeoutError or let the cancellation propagate + self._cancelling = task.cancelling() + if self._cancelled: raise asyncio.TimeoutError from None @@ -727,14 +733,18 @@ def __exit__( if self._tasks: enter_task = self._tasks.pop() - if exc_type is asyncio.CancelledError and self._cancelled: + if self._cancelled and exc_type is asyncio.CancelledError: assert enter_task is not None # The timeout was hit, and the task was cancelled # so we need to uncancel the last task that entered the context manager # since the cancellation should not leak out of the context manager if sys.version_info >= (3, 11): - enter_task.uncancel() - raise asyncio.TimeoutError from None + # If the task was already cancelling don't raise + # asyncio.TimeoutError and instead return None + # to allow the cancellation to propagate + if enter_task.uncancel() > self._cancelling: + return None + raise asyncio.TimeoutError from exc_val return None def timeout(self) -> None: diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 8e9bb29482d..debb163581a 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -444,6 +444,40 @@ async def test_timer_context_timeout_does_not_leak_upward() -> None: assert current_task.cancelling() == 0 +@pytest.mark.skipif( + sys.version_info < (3, 11), reason="Python 3.11+ is required for .cancelling()" +) +async def test_timer_context_timeout_does_swallow_cancellation() -> None: + """Verify that the TimerContext does not swallow cancellation.""" + loop = asyncio.get_running_loop() + current_task = asyncio.current_task() + ctx = helpers.TimerContext(loop) + + async def task_with_timeout(): + nonlocal ctx + current_task = asyncio.current_task() + assert current_task is not None + with pytest.raises(asyncio.TimeoutError): + with ctx: + assert current_task.cancelling() == 0 + await asyncio.sleep(1) + + task = asyncio.create_task(task_with_timeout()) + await asyncio.sleep(0) + task.cancel() + assert task.cancelling() == 1 + ctx.timeout() + + # Cancellation should not leak into the current task + assert current_task.cancelling() == 0 + # Cancellation should not be swallowed if the task is cancelled + # and it also times out + await asyncio.sleep(0) + with pytest.raises(asyncio.CancelledError): + await task + assert task.cancelling() == 1 + + def test_timer_context_no_task(loop: asyncio.AbstractEventLoop) -> None: with pytest.raises(RuntimeError): with helpers.TimerContext(loop): From b6bdb5c0f3fd1fd205ddd8e0d95249f8a662ab69 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 13:00:12 -0500 Subject: [PATCH 7/9] Update aiohttp/helpers.py --- aiohttp/helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index 9a23ecc7eab..d37864339bb 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -733,7 +733,7 @@ def __exit__( if self._tasks: enter_task = self._tasks.pop() - if self._cancelled and exc_type is asyncio.CancelledError: + if exc_type is asyncio.CancelledError and self._cancelled: assert enter_task is not None # The timeout was hit, and the task was cancelled # so we need to uncancel the last task that entered the context manager From 86c302277cb2e84eb24ccd63e26ae6377b098b19 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 13:06:03 -0500 Subject: [PATCH 8/9] lint --- tests/test_helpers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index debb163581a..dda7fe9bb4a 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -453,13 +453,13 @@ async def test_timer_context_timeout_does_swallow_cancellation() -> None: current_task = asyncio.current_task() ctx = helpers.TimerContext(loop) - async def task_with_timeout(): + async def task_with_timeout() -> None: nonlocal ctx - current_task = asyncio.current_task() - assert current_task is not None + new_task = asyncio.current_task() + assert new_task is not None with pytest.raises(asyncio.TimeoutError): with ctx: - assert current_task.cancelling() == 0 + assert new_task.cancelling() == 0 await asyncio.sleep(1) task = asyncio.create_task(task_with_timeout()) From f5cbb22c67c5f8655e7bf54c180c2603f4d61ed8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 28 Sep 2024 13:07:52 -0500 Subject: [PATCH 9/9] lint --- tests/test_helpers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index dda7fe9bb4a..33c04061084 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -451,6 +451,7 @@ async def test_timer_context_timeout_does_swallow_cancellation() -> None: """Verify that the TimerContext does not swallow cancellation.""" loop = asyncio.get_running_loop() current_task = asyncio.current_task() + assert current_task is not None ctx = helpers.TimerContext(loop) async def task_with_timeout() -> None: