search

python-trio / flake8-async

Highly opinionated linter for Trio code
https://flake8-async.readthedocs.io
MIT License
17 stars 2 forks source link

Should `ASYNC102` special-case `.aclose()`? #156

Closed Zac-HD closed 4 months ago

Zac-HD commented 1 year ago

trio.abc.AsyncResource.aclose() specifies that it must close the resource even if the method is cancelled or raises an exception; the error indicates a failure to close gracefully but the resource will still be closed.

This means that it is safe to write finally: await x.aclose() - in the no-error case it will close gracefully; and if cancelled it's equivalent to trio.aclose_forcefully(x) and will still close. Adding a cancel-shielded scope and timeout here is actually counter-productive!

So... should we special-case this method? Or perhaps rethink the check more generally? I'm not sure, but wanted to track the question. It's not an abstract question either, if we want to lint for e.g. https://github.com/encode/httpcore/issues/642#issuecomment-1459636877.

jakkdl commented 1 year ago

wow this is some complicated shit, but I've hopefully succeeded in wrapping my head around it sufficiently to have a basic grasp. Excluding it from async102 sounds correct at least, but it may definitely need more logic and/or another check to handle the various cases. Shouldn't be hard to invert the check specifically for finally: await x.aclose() and give a warning if that's inside a [cancel]-[shielded] scope.

Other than aclose or complicated teardown stuff, is the base-case async102 useful?

But yeah whatever you conclude I can almost certainly implement it, the variable- and typetracking (+me having dozens of visitors under my belt) should be powerful enough for even very weird cases, and this sounds important enough that a lot of logic is actually gonna be worth it.

Zac-HD commented 4 months ago

Fixed by #222