Add asyncio support to current checks #208

Activity

1. Zac-HD commented on Feb 25, 2024

   

   Zac-HD

   on Feb 25, 2024

   Member

   More actions

   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.

   👍React with 👍1Reacted by John Litborn
2. jakkdl commented on Feb 26, 2024

   

   jakkdl

   on Feb 26, 2024

   Last edited by jakkdl

   MemberAuthor

   More actions

   partial

   • ASYNC102: It's unsafe to await inside finally: or except BaseException/trio.Cancelled unless you use a shielded cancel scope with a timeout.
     • Awaiting inside finally/except is bad: Suggestion: ASYNC119 when using an async context manager in an async generator #211, and asyncio does have https://docs.python.org/3/library/asyncio-task.html#asyncio.shield + https://docs.python.org/3/library/asyncio-task.html#timeouts
     • something something asyncio shields are different, so they might not apply: Suggestion: ASYNC119 when using an async context manager in an async generator #211 (comment)

   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
     • suggest https://docs.python.org/3/library/asyncio-task.html#timeouts
   • 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.
3. jakkdl

   added

   good first issueGood for newcomersGood for newcomers

   on Mar 5, 2024
4. jakkdl

   added

   prioritized

   on Mar 14, 2024
5. jakkdl commented on May 16, 2024

   

   jakkdl

   on May 16, 2024

   Last edited by jakkdl

   MemberAuthor

   More actions

   • ASYNC113: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task is not an async method, so it's vulnerable to the same problem as start_soon.

   [this comment previously talked about ASYNC111, but async support for 111 was already implemented at the time of posting 😕 ]
6. jakkdl commented on May 29, 2024

   

   jakkdl

   on May 29, 2024

   MemberAuthor

   More actions

   • ASYNC113: https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task is not an async method, so it's vulnerable to the same problem as start_soon.

   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.