Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix race conditions, support Python 3.11 #295

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix race conditions, support Python 3.11 #295

wants to merge 8 commits into from

Conversation

asvetlov
Copy link
Member

No description provided.

@asvetlov
Copy link
Member Author

Python 3.11 tests fail, not released yet Python 3.11.0a6+ required

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Feb 17, 2022

If this fixes the same race conditions we were looking at before, can you put the tests back in from that discussion? Seems you reverted them for some reason: ab04eb5

@asvetlov
Copy link
Member Author

Thanks, I lost these tests.
Not sure if 1ms delay produces stable CI. Let me port tests and see how they work

@Dreamsorcerer
Copy link
Member

Thanks, I lost these tests. Not sure if 1ms delay produces stable CI. Let me port tests and see how they work

It worked 100% of the times we tried it so far. If the last assert for call_order fails, then the timing went wrong. If it fails anywhere else, then the race condition is not fixed.

@Dreamsorcerer
Copy link
Member

I think this will still fail the second test. If the timeout cancellation is followed by the explicit cancellation, then the explicit cancellation is ignored. Without fixing this, it would require a user to do something awkward everytime they cancel a task, like:

while not task.cancel():
    await asyncio.sleep(0)

I think we'll still need https://bugs.python.org/issue45098
With the new ExceptionGroup, I think my suggestion should change to raising multiple CancelledErrors. Then we can replace the timeout CancelledError with a TimeoutError and reraise any other CancelledErrors.

@asvetlov
Copy link
Member Author

@Dreamsorcerer you may be interested in CPython discussion about asyncio timeouts design: https://bugs.python.org/issue46771?@ok_message=msg%20413603%20created%0Aissue%2046771%20message_count%2C%20messages%20edited%20ok&@template=item

Please feel free to join us.
I suggest suspending asyncio-timeout upgrading until the asyncio timeout has landed.

Comment on lines +215 to +224
skip = False
if sys.version_info >= (3, 9):
# Analyse msg
assert exc_val is not None
if not exc_val.args or exc_val.args[0] != id(self):
skip = True
if not skip:
if sys.version_info >= (3, 11):
asyncio.current_task().uncancel()
raise asyncio.TimeoutError
Copy link
Member

@Dreamsorcerer Dreamsorcerer Feb 22, 2022

Choose a reason for hiding this comment

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

Following what Guido was saying, I think this needs to check to check the cancelled count, maybe something like:

Suggested change
skip = False
if sys.version_info >= (3, 9):
# Analyse msg
assert exc_val is not None
if not exc_val.args or exc_val.args[0] != id(self):
skip = True
if not skip:
if sys.version_info >= (3, 11):
asyncio.current_task().uncancel()
raise asyncio.TimeoutError
self._timeout_handler = None
if sys.version_info >= (3, 11):
if asyncio.current_task().uncancel() == 0:
self._timeout_handler = None
raise asyncio.TimeoutError()
else:
raise asyncio.TimeoutError()

I think this is all that's needed, as the == _State.TIMEOUT tells us that we initiated a cancel, so there's no need for the cancel message anymore. Would be good to test this out against those previous tests before wrapping up the cpython discussion.

timeout_cm = timeout(3600) # reschedule just before the usage
task2 = asyncio.create_task(main())
assert "ok" == await task2
assert timeout_cm.expired
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert timeout_cm.expired
assert timeout_cm.expired
async def race_condition(offset: float = 0) -> List[str]:
"""Common code for below race condition tests."""
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 pytest.warns(DeprecationWarning):
with Timeout(deadline, loop):
await asyncio.sleep(10)
call_order: List[str] = []
f_exit = log_func(Timeout._do_exit, "exit", call_order)
Timeout._do_exit = f_exit # type: ignore[assignment]
f_timeout = log_func(Timeout._on_timeout, "timeout", call_order)
Timeout._on_timeout = f_timeout # type: ignore[assignment]
loop = asyncio.get_running_loop()
deadline = loop.time() + 1
t = asyncio.create_task(test_task(deadline, loop))
loop.call_at(deadline + offset, log_func(t.cancel, "cancel", call_order))
# If we get a TimeoutError, then the code is broken.
with pytest.raises(asyncio.CancelledError):
await t
return call_order
@pytest.mark.skipif(sys.version_info < (3, 11), reason="Only fixed in 3.11+")
@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.
"""
call_order = await race_condition()
# This test is very timing dependant, so we check the order that calls
# happened to be sure the test itself ran correctly.
assert call_order == ["cancel", "timeout", "exit"]
@pytest.mark.skipif(sys.version_info < (3, 11), reason="Only fixed 3.11+.")
@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.
"""
call_order = await race_condition(0.000001)
# This test is very timing dependant, so we check the order that calls
# happened to be sure the test itself ran correctly.
assert call_order == ["timeout", "cancel", "exit"]

sileht added a commit to sileht/redis-py that referenced this pull request Mar 2, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
redis#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
sileht added a commit to sileht/redis-py that referenced this pull request Mar 2, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
redis#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
sileht added a commit to sileht/redis-py that referenced this pull request Mar 2, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
redis#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
dvora-h pushed a commit to redis/redis-py that referenced this pull request Mar 16, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants