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

Add asyncio support to current checks #208

Open
jakkdl opened this issue Feb 25, 2024 · 4 comments
Open

Add asyncio support to current checks #208

jakkdl opened this issue Feb 25, 2024 · 4 comments
Labels

Comments

@jakkdl
Copy link
Member

jakkdl commented Feb 25, 2024

  • Go through all errors and see which ones are applicable to asyncio
    • Add support & parametrize tests for them
  • Update README.md to indicate which tests are available for which library (asyncio, anyio, trio)

Could also consider supporting curio or any other IO library.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 25, 2024

I don't think we should support other libraries - curio is no longer being updated, twisted is more callback than async, others are even smaller.

@jakkdl
Copy link
Member Author

jakkdl commented Feb 26, 2024

partial

todo

  • ASYNC110: while <condition>: await trio.sleep() should be replaced by a trio.Event.
    • I'm assuming there's a construct for this
    • yes there is, asyncio.Event
  • ASYNC109: Async function definition with a timeout parameter - use trio.[fail/move_on]_[after/at] instead
  • ASYNC100: A with trio.fail_after(...): or with trio.move_on_after(...):
    context does not contain any await statements. This makes it pointless, as
    the timeout can only be triggered by a checkpoint.
    • asyncio equivalents: asyncio.timeout, asyncio.timeout_at,
  • ASYNC116: trio.sleep() with >24 hour interval should usually be trio.sleep_forever().
    • there's no asyncio.sleep_forever() as far as I can tell.
  • The only blocker for autofixing 9xx in asyncio is some minor test infrastructure, as we can insert asyncio.sleep(0) instead of library.lowlevel.checkpoint(). [No longer blocked by test infra]

tracked in #215

  • ASYNC111: Variable, from context manager opened inside nursery, passed to start[_soon] might be invalidly accessed while in use, due to context manager closing before the nursery. This is usually a bug, and nurseries should generally be the inner-most context manager.
  • ASYNC112: Nursery body with only a call to nursery.start[_soon] and not passing itself as a parameter can be replaced with a regular function call.
  • ASYNC101: yield inside a nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling.

@jakkdl jakkdl mentioned this issue Feb 26, 2024
4 tasks
@jakkdl jakkdl added the good first issue Good for newcomers label Mar 5, 2024
@jakkdl
Copy link
Member Author

jakkdl commented May 16, 2024

[this comment previously talked about ASYNC111, but async support for 111 was already implemented at the time of posting 😕 ]

@jakkdl
Copy link
Member Author

jakkdl commented May 29, 2024

One part that leaves me somewhat confused about implementing ASYNC113 is what, if anything, to do about ASYNC114. Currently we gate ASYNC113 to only warn on startable methods, presumably to reduce false positives when you don't care about the background task being properly up and running. But I don't think there's any way to differentiate those for asyncio tasks, so we're left with the options of:

  1. Always warn on all calls
  2. Only warn for calls specified in startable_in_context_manager.

If startable_in_context_manager was expanded to support wildcards, we could get the best of both worlds by telling asyncio users to specify * if they want the former behaviour. (this would also allow anyio/trio users to go maximum defensiveness, in case they're starting functions not defined in code that could trigger ASYNC114, or they might've forgotten to add task_status to functions)

Another problem is that since there's no .start()-equivalent that guarantees that the task has been started, there's no standardized way of alleviating the problem that we could detect, so users are left to handle it in whatever way they can, and then noqa the warning.

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

No branches or pull requests

2 participants