Skip to content
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

ASYNC120: unintuitive error message #272

Open
richardsheridan opened this issue Jul 6, 2024 · 2 comments
Open

ASYNC120: unintuitive error message #272

richardsheridan opened this issue Jul 6, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@richardsheridan
Copy link

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.

@Zac-HD Zac-HD added the bug Something isn't working label Jul 6, 2024
@jakkdl
Copy link
Member

jakkdl commented Jul 7, 2024

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 jakkdl added enhancement New feature or request and removed bug Something isn't working labels Jul 16, 2024
@jakkdl jakkdl changed the title False alarm for ASYNC120 ASYNC120: unintuitive error message Jul 16, 2024
@jakkdl
Copy link
Member

jakkdl commented Jul 17, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants