search

python-trio / flake8-async

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

ASYNC120: unintuitive error message #272

Open richardsheridan opened 2 weeks ago

richardsheridan commented 2 weeks ago

I recently bumped deps for trio-parallel and I got an ASYNC120:

https://github.com/richardsheridan/trio-parallel/actions/runs/9820324879/job/27114776180?pr=422#step:6:13

Which is here:

https://github.com/richardsheridan/trio-parallel/blob/7ccb4b75f36148952a6fec90bb94f3335dd8db2d/trio_parallel/_proc.py#L109-L117

Because this checkpoint is shielded, a Cancelled cannot be raised out of it to overwrite the exception. I feel this is a bug of some kind because there are nearby shielded checkpoints in exceptions that don't raise this warning.

jakkdl commented 2 weeks ago

I'm pretty sure this is because you have a shielded cancelscope, but no timeout on it. Both 102 & 120 requires both shield&timeout.

I could perhaps change the error message to be more informative when there is a cancelscope with only one of shield/timeout

jakkdl commented 4 days ago

Thinking at this a bit more, we maybe instead want a new rule for shielded-cancelscope-with-no-timeout? The error that 102/await-in-finally-or-cancelled and 120/await-in-except are warning about only requires the shield to protect against, and the reason for the timeout is entirely separate (not wanting the function to stall/hang).

I don't know if we'd want to give an error if any shielded cancelscope ever has no timeout, or if we want to isolate it to scopes in critical sections (i.e. where 102/120 would otherwise trigger).