Detect asyncio when the loop is running without a Task - #59
Conversation
Twisted's asyncioreactor (and call_soon callbacks) run on the asyncio loop without an asyncio.Task, so current_task() is None and sniffio raised AsyncLibraryNotFoundError.
fallenmi
left a comment
There was a problem hiding this comment.
Please make the regression test fail promptly when the fallback is absent. In sync_cb(), Python evaluates current_async_library() before loop.stop(). On the exact base, that call raises AsyncLibraryNotFoundError; asyncio reports the callback exception, skips loop.stop(), and run_forever() then waits indefinitely. I reproduced this event-loop behavior with a stdlib-only subprocess: the callback ran, but the process did not terminate. Put loop.stop() in a finally block so the assertion outside the callback produces a deterministic failure on the red path.
Please also add newsfragments/51.bugfix.rst. CONTRIBUTING.md delegates to the Trio contribution guide, which says every pull request with a user-visible effect should add a newsfragment; this is the user-visible bugfix for #51.
I verified this against exact head 0cbbda24fb2919c21e803580c4c5fe447bab3fed and current merge a9b862f6d73b6afefc39704de56efcf22c8c8df1. The production fallback itself is reasonable, and the live CI is fully green: 20/20 check runs, 1/1 check suite, 1/1 Actions workflow, and the Read the Docs status.
Disclosure: this review was prepared with Codex assistance; I independently verified the exact refs, source path, test behavior, repository policy, interactions, and live CI before submission.
|
Updated: |
fallenmi
left a comment
There was a problem hiding this comment.
The two requested changes are present on exact head 98a876b4f8a28a168a61172b5fd81c65e7b1f114. loop.stop() now runs in a finally, so a detection failure still exits run_forever() and the outer assertion fails deterministically; newsfragments/51.bugfix.rst is also present.
I rechecked the full production path and current merge: thread-local/context-variable precedence and existing Task detection remain intact, the fallback only recognizes a running asyncio loop without a Task, and all three touched blobs are identical between head and merge. Live CI is fully green: 20/20 checks, the suite, workflow, and Read the Docs status. I found no remaining blocker.
Disclosure: I used OpenAI Codex and Claude Sonnet to assist this rereview; I verified the exact refs, old-to-new delta, failure-path behavior, production semantics, repository policy and interactions, merge blobs, and live CI before submission.
|
This is a duplicate of #39, and with a worse implementation: there is no point in trying CPython 3.10.21 (asyncio REPL): >>> import asyncio
>>> from timeit import timeit
>>> timeit(asyncio.current_task)
0.6053914040094241
>>> timeit(asyncio.get_running_loop)
0.46340504300314933
>>> import sniffio # python-trio/sniffio
>>> import sniffio39 # python-trio/sniffio#39
>>> import sniffio59 # python-trio/sniffio#59
>>> timeit(sniffio.current_async_library)
Traceback (most recent call last):
File "/home/user/.local/share/mise/installs/python/3.10.21/lib/python3.10/concurrent/futures/_base.py", line 458, in result
return self.__get_result()
File "/home/user/.local/share/mise/installs/python/3.10.21/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
raise self._exception
File "/home/user/.local/share/mise/installs/python/3.10.21/lib/python3.10/asyncio/__main__.py", line 34, in callback
coro = func()
File "<console>", line 1, in <module>
File "/home/user/.local/share/mise/installs/python/3.10.21/lib/python3.10/timeit.py", line 234, in timeit
return Timer(stmt, setup, timer, globals).timeit(number)
File "/home/user/.local/share/mise/installs/python/3.10.21/lib/python3.10/timeit.py", line 178, in timeit
timing = self.inner(it, self.timer)
File "<timeit-src>", line 6, in inner
File "/home/user/.local/lib/python3.10/site-packages/sniffio/_impl.py", line 93, in current_async_library
raise AsyncLibraryNotFoundError(
sniffio._impl.AsyncLibraryNotFoundError: unknown async library, or not in async context
>>> timeit(sniffio39.current_async_library)
0.8540417910553515
>>> timeit(sniffio59.current_async_library)
1.519261592067778 |
current_task() already raises outside a loop, so probing it first is strictly slower and still needs the get_running_loop fallback for call_soon callbacks.
|
You're right — This PR is still the narrow bugfix with the |
See #38. It is about semantics. Your PR changes that very same semantics. And #39 addresses the very same issue you describe at the beginning of this PR (#35). To clarify, See #35 (comment). And as you can see, the documentation refers to coroutines, which would no longer correspond to the changed semantics. |
|
However, it is worth noting that AnyIO and Trio already have semantics that differ from those described in the sniffio documentation: >>> import sniffio
>>> def test():
... try:
... print(sniffio.current_async_library())
... except sniffio.AsyncLibraryNotFoundError:
... print("unknown")
>>> import asyncio
>>> async def asyncio_test():
... asyncio.get_running_loop().call_soon(test)
>>> import trio
>>> async def trio_test():
... trio.lowlevel.current_trio_token().run_sync_soon(test)
>>> import anyio
>>> asyncio.run(asyncio_test())
unknown
>>> anyio.run(asyncio_test)
asyncio
>>> trio.run(trio_test)
trioOn the one hand, one could argue that the documentation refers to coroutines only in the context of adding support for a new library, and says nothing about the behavior of |
|
You're right that this changes the documented "must be inside a Task" reading — that's the bug. #39 can still land the docs/ |
|
Oh please shut up Claude. @x42005e1f interesting! idk what #39 needs from now but I think it's a good idea, especially if as you note the definition of what is "running" is already different between Trio and asyncio. |
current_task() is None for Twisted's asyncioreactor and for call_soon callbacks, so sniffio treated that as "not in async context" even though asyncio.get_running_loop() succeeds.
If there is no current Task, fall back to get_running_loop() before giving up.
Fixes #51