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

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

Closed
Zac-HD opened this issue Mar 14, 2023 · 2 comments
Closed

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

Zac-HD opened this issue Mar 14, 2023 · 2 comments

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Mar 14, 2023

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. encode/httpcore#642 (comment).

@jakkdl
Copy link
Member

jakkdl commented Mar 14, 2023

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 Zac-HD changed the title Should TRIO102 special-case .aclose()? Should ASYNC102 special-case .aclose()? Mar 13, 2024
@Zac-HD
Copy link
Member Author

Zac-HD commented Mar 22, 2024

Fixed by #222

@Zac-HD Zac-HD closed this as completed Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants