-
Notifications
You must be signed in to change notification settings - Fork 59
Don't raise if the task already have been cancelled explicitly #237
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,4 +89,4 @@ ENV/ | |
.ropeproject | ||
|
||
.mypy_cache | ||
.pytest_cache | ||
.pytest_cache |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
Placeholder | ||
Placeholder |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
isort==5.9.3 | ||
-e . | ||
black==21.8b0; implementation_name=="cpython" | ||
pytest==6.2.5 | ||
pytest-asyncio==0.15.1 | ||
pytest-cov==2.12.1 | ||
docutils==0.17.1 | ||
mypy==0.910; implementation_name=="cpython" | ||
flake8==3.9.2 | ||
isort==5.9.3 | ||
mypy==0.910; implementation_name=="cpython" | ||
pre-commit==2.15 | ||
-e . | ||
pytest==6.2.5 | ||
pytest-asyncio==0.15.1 | ||
pytest-cov==2.12.1 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -373,9 +373,9 @@ async def test_task(deadline: float, loop: asyncio.AbstractEventLoop) -> None: | |||||
await t | ||||||
|
||||||
|
||||||
@pytest.mark.xfail( | ||||||
reason="The test is CPU performance sensitive, might fail on slow CI box" | ||||||
) | ||||||
# @pytest.mark.xfail( | ||||||
# reason="The test is CPU performance sensitive, might fail on slow CI box" | ||||||
# ) | ||||||
@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: | ||||||
|
@@ -396,7 +396,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 + 0.0000000000001, t.cancel) | ||||||
loop.call_at(deadline + 0.00000000001, t.cancel) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think you made the value so small that you've tricked your system into passing the tests by running the cancel() before the previous task. The value needs to be big enough to ensure the task is scheduled after the timeout task, but small enough that it happens immediately after without a chance to run the This test should be reliably failing. To verify the conditions, add prints to see when this cancel happens in relation to the timeout actions. The execution should be: I suspect on your machine you've managed to change the execution order so |
||||||
# If we get a TimeoutError, then the code is broken. | ||||||
with pytest.raises(asyncio.CancelledError): | ||||||
await t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this much harder to reason about, I think it's more risky than the other solutions.
The reason this somewhat works is that it changes the call order in the before case to:
-> t.cancel()
-> Timeout._on_timeout()
-> Timeout._on_exit()
-> Timeout._cancel() # This is a noop, as it's already exited, so only 1 cancellation happens.
I'm not really sure that provides any guarantees (particularly when you have a more complex project with many tasks in flight), as it's just delaying the cancellation, which happens to be after the exit has already happened.
In the after case, this is not fixed at all (just like all the other attempts), in this case the execution looks like:
-> Timeout._on_timeout()
-> t.cancel()
-> Timeout._cancel() # Again, we've triggered 2 cancellations in a row
-> Timeout._do_exit() # No way to know there were 2 cancellations.