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

Don't raise if the task already have been cancelled explicitly #237

Closed
wants to merge 2 commits into from

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Sep 4, 2021

Fixes #229

@asvetlov
Copy link
Member Author

asvetlov commented Sep 4, 2021

I'll try to manage the red CI on evening. Tests pass locally.

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Sep 4, 2021

CI is failing because this unfortunately still does not fix the second case. A cancellation can still be suppressed.

@Dreamsorcerer
Copy link
Member

I feel like there needs to be something new introduced to asyncio, so when looking at the CancelledError you can tell how many times cancel() was called before raising the exception and/or whether all cancellations were caused by us (i.e. was the sentinel sent for every cancellation).

Maybe the args on the exception could contain every msg from every cancel(). So, in these edge cases it would look like e.args == (None, _SENTINEL) from which we can see that there were 2 cancellations and only 1 was from our timeout. Then we can do a simple check like if all(msg is _SENTINEL for msg in e.args).

return
task.cancel()
self._state = _State.TIMEOUT
self._loop.call_soon(self._cancel)
Copy link
Member

@Dreamsorcerer Dreamsorcerer Sep 4, 2021

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.

@Dreamsorcerer
Copy link
Member

Raised this as a proposal: https://bugs.python.org/issue45098

@@ -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)
Copy link
Member

@Dreamsorcerer Dreamsorcerer Sep 4, 2021

Choose a reason for hiding this comment

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

Suggested change
loop.call_at(deadline + 0.00000000001, t.cancel)
loop.call_at(deadline + 0.000001, t.cancel)

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 __exit__().

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:
-> Timeout._on_timeout()
-> t.cancel()
-> Timeout._cancel()
-> Timeout._do_exit()

I suspect on your machine you've managed to change the execution order so t.cancel() happens first, which is what the previous test is for.

@asvetlov
Copy link
Member Author

Version 5.0+ wiorks exactly as asyncio built-in does, nothing to do here.

@asvetlov asvetlov closed this Oct 31, 2024
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.

aiohttp swallows asyncio.CancelledError during connection timeout
2 participants