-
Notifications
You must be signed in to change notification settings - Fork 59
Tests and partial fix for suppressing cancellations. #230
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
Conversation
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.
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 |
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.
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.
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 think putting a 3.9+ fix in would be acceptable, as I don't see a good way to do this otherwise.
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
===========================================
- Coverage 100.00% 98.13% -1.87%
===========================================
Files 1 1
Lines 105 107 +2
Branches 15 16 +1
===========================================
Hits 105 105
- Misses 0 1 +1
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
7e136a2
to
e7d4620
Compare
Ok, let me merge and make a new release |
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)