Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Tests and partial fix for suppressing cancellations. #230

Merged
merged 10 commits into from
Sep 3, 2021
3 changes: 3 additions & 0 deletions async_timeout/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,9 @@ 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
Copy link
Member

@asvetlov asvetlov Sep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hack looks dangerous and potentially not future compatible; while I don't see a better solution.
For Python 3.9+ task.cancel(SENTINEL) variant can be used though to filter out all cancellation events not sent by async_timeout library itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think putting a 3.9+ fix in would be acceptable, as I don't see a good way to do this otherwise.

return
task.cancel()
self._state = _State.TIMEOUT

Expand Down
55 changes: 54 additions & 1 deletion tests/test_timeout.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import asyncio
import sys
import time

import pytest

from async_timeout import timeout, timeout_at
from async_timeout import Timeout, timeout, timeout_at


@pytest.mark.asyncio
Expand Down Expand Up @@ -344,3 +345,55 @@ async def test_deprecated_with() -> None:
with pytest.warns(DeprecationWarning):
with timeout(1):
await asyncio.sleep(0)


@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.

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: 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)

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.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.

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: 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)

loop = asyncio.get_running_loop()
deadline = loop.time() + 1
t = asyncio.create_task(test_task(deadline, loop))
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):
await t