From 59b155fc38f1286a3cb91ad5782d131c0b68de40 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 20:44:20 +0100 Subject: [PATCH 1/9] Setup tests and partial fix for overruling cancellations. --- async_timeout/__init__.py | 2 ++ tests/test_timeout.py | 46 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/async_timeout/__init__.py b/async_timeout/__init__.py index 6222654..2d642cb 100644 --- a/async_timeout/__init__.py +++ b/async_timeout/__init__.py @@ -198,6 +198,8 @@ def _do_exit(self, exc_type: Type[BaseException]) -> None: return None def _on_timeout(self, task: "asyncio.Task[None]") -> None: + if task._fut_waiter and task._fut_waiter.cancelled(): + return task.cancel() self._state = _State.TIMEOUT diff --git a/tests/test_timeout.py b/tests/test_timeout.py index e76c54c..90860a3 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -3,7 +3,7 @@ import pytest -from async_timeout import timeout, timeout_at +from async_timeout import Timeout, timeout, timeout_at @pytest.mark.asyncio @@ -344,3 +344,47 @@ async def test_deprecated_with() -> None: with pytest.warns(DeprecationWarning): with timeout(1): await asyncio.sleep(0) + + +@pytest.mark.asyncio +async def test_race_condition_cancel_before(): + """Test race condition when cancelling before timeout. + + If cancel happens immediately before the timeout, then + the timeout may overrule the cancellation, making it + impossible to cancel some tasks. + """ + async def test_task(deadline, loop): + with Timeout(deadline, loop): + await asyncio.sleep(10) + + loop = asyncio.get_running_loop() + deadline = loop.time() + 1 + t = asyncio.create_task(test_task(deadline, loop)) + loop.call_at(deadline, t.cancel) + await asyncio.sleep(1.1) + # If we get a TimeoutError, then the code is broken. + with pytest.raises(asyncio.CancelledError): + await t + + +@pytest.mark.asyncio +async def test_race_condition_cancel_after(): + """Test race condition when cancelling after timeout. + + Similarly to the previous test, if a cancel happens + immediately after the timeout (but before the __exit__), + then the explicit cancel can get overruled again. + """ + async def test_task(deadline, loop): + with Timeout(deadline, loop): + await asyncio.sleep(10) + + loop = asyncio.get_running_loop() + deadline = loop.time() + 1 + t = asyncio.create_task(test_task(deadline, loop)) + loop.call_at(deadline + .000001, t.cancel) + await asyncio.sleep(1.1) + # If we get a TimeoutError, then the code is broken. + with pytest.raises(asyncio.CancelledError): + await t From 6692999ffa3d45e232ec3b857ffcfc954acc38f3 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 20:56:32 +0100 Subject: [PATCH 2/9] Fix mypy errors. --- async_timeout/__init__.py | 2 +- tests/test_timeout.py | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/async_timeout/__init__.py b/async_timeout/__init__.py index 2d642cb..97ffbdc 100644 --- a/async_timeout/__init__.py +++ b/async_timeout/__init__.py @@ -198,7 +198,7 @@ def _do_exit(self, exc_type: Type[BaseException]) -> None: return None def _on_timeout(self, task: "asyncio.Task[None]") -> None: - if task._fut_waiter and task._fut_waiter.cancelled(): + if task._fut_waiter and task._fut_waiter.cancelled(): # type: ignore[attr-defined] return task.cancel() self._state = _State.TIMEOUT diff --git a/tests/test_timeout.py b/tests/test_timeout.py index 90860a3..3c0e83e 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -1,5 +1,6 @@ import asyncio import time +import sys import pytest @@ -346,15 +347,16 @@ async def test_deprecated_with() -> None: await asyncio.sleep(0) +@pytest.mark.skipif(sys.version_info < (3, 7)) @pytest.mark.asyncio -async def test_race_condition_cancel_before(): +async def test_race_condition_cancel_before() -> None: """Test race condition when cancelling before timeout. If cancel happens immediately before the timeout, then the timeout may overrule the cancellation, making it impossible to cancel some tasks. """ - async def test_task(deadline, loop): + async def test_task(deadline: float, loop: asyncio.Loop) -> None: with Timeout(deadline, loop): await asyncio.sleep(10) @@ -368,15 +370,16 @@ async def test_task(deadline, loop): await t +@pytest.mark.skipif(sys.version_info < (3, 7)) @pytest.mark.asyncio -async def test_race_condition_cancel_after(): +async def test_race_condition_cancel_after() -> None: """Test race condition when cancelling after timeout. Similarly to the previous test, if a cancel happens immediately after the timeout (but before the __exit__), then the explicit cancel can get overruled again. """ - async def test_task(deadline, loop): + async def test_task(deadline: float, loop: asyncio.Loop) -> None: with Timeout(deadline, loop): await asyncio.sleep(10) From 6349c70c4482bba23fe1cbb801f6db1755b3815e Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 21:23:23 +0100 Subject: [PATCH 3/9] Fix --- tests/test_timeout.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_timeout.py b/tests/test_timeout.py index 3c0e83e..78cf36e 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -356,7 +356,7 @@ async def test_race_condition_cancel_before() -> None: the timeout may overrule the cancellation, making it impossible to cancel some tasks. """ - async def test_task(deadline: float, loop: asyncio.Loop) -> None: + async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: with Timeout(deadline, loop): await asyncio.sleep(10) @@ -379,7 +379,7 @@ async def test_race_condition_cancel_after() -> None: immediately after the timeout (but before the __exit__), then the explicit cancel can get overruled again. """ - async def test_task(deadline: float, loop: asyncio.Loop) -> None: + async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: with Timeout(deadline, loop): await asyncio.sleep(10) From 7522bfe6e0a51329b65849b99767a0cee0343667 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 21:30:35 +0100 Subject: [PATCH 4/9] Imports --- tests/test_timeout.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_timeout.py b/tests/test_timeout.py index 78cf36e..90ea372 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -1,6 +1,6 @@ import asyncio -import time import sys +import time import pytest From 24e60bc91ff2f7f31c94a3d144187500a0166625 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 21:34:02 +0100 Subject: [PATCH 5/9] Black --- tests/test_timeout.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_timeout.py b/tests/test_timeout.py index 90ea372..78056cd 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -356,6 +356,7 @@ async def test_race_condition_cancel_before() -> None: the timeout may overrule the cancellation, making it impossible to cancel some tasks. """ + async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: with Timeout(deadline, loop): await asyncio.sleep(10) @@ -379,6 +380,7 @@ async def test_race_condition_cancel_after() -> None: immediately after the timeout (but before the __exit__), then the explicit cancel can get overruled again. """ + async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: with Timeout(deadline, loop): await asyncio.sleep(10) @@ -386,7 +388,7 @@ async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: loop = asyncio.get_running_loop() deadline = loop.time() + 1 t = asyncio.create_task(test_task(deadline, loop)) - loop.call_at(deadline + .000001, t.cancel) + loop.call_at(deadline + 0.000001, t.cancel) await asyncio.sleep(1.1) # If we get a TimeoutError, then the code is broken. with pytest.raises(asyncio.CancelledError): From 00b02a66d59d7807632b7e333e25fab926167091 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 21:36:30 +0100 Subject: [PATCH 6/9] Flake8 --- async_timeout/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/async_timeout/__init__.py b/async_timeout/__init__.py index 97ffbdc..efdac3c 100644 --- a/async_timeout/__init__.py +++ b/async_timeout/__init__.py @@ -198,7 +198,7 @@ def _do_exit(self, exc_type: Type[BaseException]) -> None: return None def _on_timeout(self, task: "asyncio.Task[None]") -> None: - if task._fut_waiter and task._fut_waiter.cancelled(): # type: ignore[attr-defined] + if task._fut_waiter and task._fut_waiter.cancelled(): # type: ignore[attr-defined] # noqa: E501 return task.cancel() self._state = _State.TIMEOUT From 634b4b320b6d27e730e2931d12bcd58d4a76f470 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Aug 2021 21:39:07 +0100 Subject: [PATCH 7/9] Pytest --- tests/test_timeout.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_timeout.py b/tests/test_timeout.py index 78056cd..cf8f30d 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -347,7 +347,7 @@ async def test_deprecated_with() -> None: await asyncio.sleep(0) -@pytest.mark.skipif(sys.version_info < (3, 7)) +@pytest.mark.skipif(sys.version_info < (3, 7), reason="Not supported in 3.6") @pytest.mark.asyncio async def test_race_condition_cancel_before() -> None: """Test race condition when cancelling before timeout. @@ -371,7 +371,7 @@ async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: await t -@pytest.mark.skipif(sys.version_info < (3, 7)) +@pytest.mark.skipif(sys.version_info < (3, 7), reason="Not supported in 3.6") @pytest.mark.asyncio async def test_race_condition_cancel_after() -> None: """Test race condition when cancelling after timeout. From e7d4620bc5710c1aa5128fe571becff521e31872 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 2 Sep 2021 20:16:07 +0100 Subject: [PATCH 8/9] Add comments about using Timeout class directly. --- tests/test_timeout.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_timeout.py b/tests/test_timeout.py index cf8f30d..131c94d 100644 --- a/tests/test_timeout.py +++ b/tests/test_timeout.py @@ -358,6 +358,8 @@ async def test_race_condition_cancel_before() -> None: """ async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: + # We need the internal Timeout class to specify the deadline (not delay). + # This is needed to create the precise timing to reproduce the race condition. with Timeout(deadline, loop): await asyncio.sleep(10) @@ -382,6 +384,8 @@ async def test_race_condition_cancel_after() -> None: """ async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: + # We need the internal Timeout class to specify the deadline (not delay). + # This is needed to create the precise timing to reproduce the race condition. with Timeout(deadline, loop): await asyncio.sleep(10) From 53ee7a9b8ea881a359663975823e4446b69c6584 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Fri, 3 Sep 2021 14:12:07 +0300 Subject: [PATCH 9/9] Update async_timeout/__init__.py --- async_timeout/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/async_timeout/__init__.py b/async_timeout/__init__.py index efdac3c..31a4aef 100644 --- a/async_timeout/__init__.py +++ b/async_timeout/__init__.py @@ -198,6 +198,7 @@ def _do_exit(self, exc_type: Type[BaseException]) -> None: return None def _on_timeout(self, task: "asyncio.Task[None]") -> None: + # See Issue #229 and PR #230 for details if task._fut_waiter and task._fut_waiter.cancelled(): # type: ignore[attr-defined] # noqa: E501 return task.cancel()