-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
ASYNC100 now treats start_soon() as a cancel point #327
Conversation
flake8_async/visitors/visitor91x.py
Outdated
def visit_Call(self, node: cst.Call) -> None: | ||
# [Nursery/TaskGroup].start_soon introduces a cancel point | ||
if ( | ||
isinstance(node.func, cst.Attribute) | ||
and isinstance(node.func.value, cst.Name) | ||
and node.func.attr.value == "start_soon" | ||
and node.func.value.value in self.taskgroups | ||
): | ||
self.checkpoint_cancel_point() |
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 don't think this is quite correct; .start_soon(...)
is not itself a cancel point, but rather it means that Nursery.__aexit__
will be a cancel point.
(unless there is an earlier checkpoint in the nursery block, during which the task finishes - in which case we can rely on that checkpoint instead)
It's a pretty subtle distinction but we should be careful about that in both implementation and docs.
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.
Ah good catch, I thought it wouldn't make a difference to the implementation at first but turns out it does. Fixed~
Upon further consideration a rule for "redundant" cancel scopes would probably be quite tricky, and only be applicable if the potentially redundant scope has no timeout or shielding, and is not getting manually canceled, etc etc. So probably not worth considering |
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.
once more with feeling!
unreverts #317 (partially reverted in #326) and fixes #325