Repository navigation
Warn on finally: cancel_scope.cancel() #365
Description
Like async102 and async120, this is to address the concerns discussed in python-trio/trio#455. (also: should the rules docs link there?)
with CancelScope() as scope:
try:
... # whatever sync and async code here
finally:
scope.cancel()
# You'd hope this just stops any ongoing work, but it swallows exceptions too!In most cases the solution is simply to scope.cancel() without the try/finally block - structured concurrency means we reliably clean up on exceptions anyway. For some, the cancel scope is entirely unnecessary, and you could call e.g. recv_chan.close() instead of cancelling.
Activity
gpshead commentedon Apr 1, 2025
This feels ironically like a harder for a human to notice version of the https://peps.python.org/pep-0765/ return-within-finally anti-pattern. 😅
jakkdl commentedon Apr 1, 2025
hmm, that's a weirdly indirect rule. The proper fix here would of course be that everybody writes correct
__aexit__handlers, but given that python-trio/trio#1559 remains open and__aexit__generally being insanely difficult I suppose we might as well add another layer of redundancy with this.Zac-HD commentedon Apr 1, 2025
I think specifically for
AsyncResourceI'd be happier with proposal (2) of python-trio/trio#1559 (comment), but this isn't limited toAsyncResource- as in the top post of this issue, you can get the same effect with rawCancelScope, and as in that comment by @njsmith I don't like teleporting exceptions around.Hmm. The original motivation had an async context manager between the cancel scope and the try/finally; I think you actually need that to trigger my problem, because as you note it's really about
__aexit__-while-cancelled. So thefinally:is a bit of a red herring; what we actually care about is cancelling when an exception is propagating and going to hit an__aexit__.Maybe we should work out an endorsed design for
__aexit__instead, and how to lint for that?jakkdl commentedon Apr 1, 2025
yeah it took me a second to figure out how your repro could ever cause swallowed exceptions, and I do indeed think you need
__aexit__We have @agronholm's recent suggestion in the internal repo, so if we get a version of that into trio & anyio we could tell users to use mix-in +
__acontextmanager__instead.Trying to detect if an
__aexit__follows an endorsed design sounds non-trivial, but warning on all uses of it would be very noisy, and it's a non-trivial rewrite for the user that I don't think we could offer an autofix for, so it might have to start out as an optional rule.