Tests and partial fix for suppressing cancellations. by Dreamsorcerer · Pull Request #230 · aio-libs/async-timeout (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits10 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Partially fixes #229.
I don't really see any way to get enough information to fully fix this, not really sure how best to proceed.
For an overview of why this is a serious problem, see: #229 (comment)
Setup tests and partial fix for overruling cancellations. Tests and partial fix for suppressing cancellations.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| @@ -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(): # type: ignore[attr-defined] # noqa: E501 |
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.
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.
Ok, let me merge and make a new release
2 participants
