Support Trio with httpx #263
Conversation
The main limitation is that sniffing isn't supported, as the way it's currently designed (starting a task in the background and never collecting it) is not compatible with structured concurrency.
| ) | ||
|
|
||
| async def sniff(self, is_initial_sniff: bool = False) -> None: # type: ignore[override] | ||
| if sniffio.current_async_library() != "asyncio": |
There was a problem hiding this comment.
Wouldn't it be more appropriate here to show the error when current_async_library == "trio", since we know for a fact that sniffing does not work with it? We don't know if sniffing works or not with other async libraries, I assume. And also, this function raises an exception when it cannot recognize which library is in use. For backwards compatibility maybe we should allow the sniffing in such a case?
There was a problem hiding this comment.
What other async library?
- I haven't seen https://github.com/dabeaz/curio used anywhere, and it isn't making new releases anyway.
- Twisted isn't supported by sniffio or anyio
- trio-asyncio never really took off, and switching to
current_async_library == triowon't make any difference
I wouldn't see a different library making the same mistakes as asyncio, ie. allowing background tasks. Anyways, Trio has shown how hard it is for a different async library to take off, even with superior design, excellent docs, etc. So, for me, the condition makes sense: today, we only support sniffing with asyncio.
I find it interesting how this is exactly the discussion we're having in elastic/elasticsearch-py#3103 (comment).
| return # Call at most once! | ||
|
|
||
| self._async_library = sniffio.current_async_library() | ||
| if self._async_library != "asyncio": |
There was a problem hiding this comment.
Same as above here. This is unlikely, but let's say that someone using "curio" was able to make sniffing work before. This would prevent them from using this feature. I think the check should be for == "trio", which is the one case that we know fails.
There was a problem hiding this comment.
Indeed, curio allows daemon tasks: https://curio.readthedocs.io/en/latest/reference.html#spawn. But I'm not aware of any HTTP client that supports curio. Since AnyIO dropped support for Curio, even asks does not support it. HTTPX does not support it.
tests/async_/test_async_transport.py
Outdated
| "sniff_callback": sniff_callback, | ||
| } | ||
|
|
||
| print(kwargs) |
There was a problem hiding this comment.
Is this print intended or left over from a debug session?
miguelgrinberg
left a comment
miguelgrinberg
left a comment
There was a problem hiding this comment.
LGTM, but I've noted that it is probably safe to check for trio to disable sniffing than to check for anything that is not asyncio.
* Support Trio on httpx backend The main limitation is that sniffing isn't supported, as the way it's currently designed (starting a task in the background and never collecting it) is not compatible with structured concurrency. * Fix lint * Add trio to develop dependencies * Remove anyio from runtime dependencies * Clarify asyncio/trio support * Link to HTTPX issue in badssl tests * Remove debug print * Reverse asyncio/trio check per review feedback (cherry picked from commit 33c0af3)
* Support Trio on httpx backend The main limitation is that sniffing isn't supported, as the way it's currently designed (starting a task in the background and never collecting it) is not compatible with structured concurrency. * Fix lint * Add trio to develop dependencies * Remove anyio from runtime dependencies * Clarify asyncio/trio support * Link to HTTPX issue in badssl tests * Remove debug print * Reverse asyncio/trio check per review feedback (cherry picked from commit 33c0af3) Co-authored-by: Quentin Pradet <[email protected]>
2 participants
The main limitation is that sniffing isn't supported, as the way it's currently designed (starting a task in the background and never collecting it) is not compatible with structured concurrency.