ASGI: Wait for response to complete before sending disconnect message #919
Conversation
|
|
||
| if typing.TYPE_CHECKING: # pragma: no cover | ||
| import asyncio | ||
| import trio |
There was a problem hiding this comment.
Okay, so trio might not be installed.
Perhaps we should be doing something like this?...
try:
import trio
Event = typing.Union[asyncio.Event, trio.Event]
except ImportError:
Event = asyncio.EventAlso it's not obvious to me that we need to bother with the if typing.TYPE_CHECKING: dance? We usually only use that if it's required to avoid otherwise circular imports.
There was a problem hiding this comment.
I tried what you're suggesting but unfortunately run into an error like:
error: Cannot assign multiple types to name "Event" without an explicit "Type[...]" annotation
Seems like type aliases need to be statically resolvable: https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
There was a problem hiding this comment.
Okay. We can't rely on the trio package necessarily being installed tho.
(Even if we're limiting this to if typing.TYPE_CHECKING:)
One other option might be to just under-specify the type in this case, as a lazy typing.Any. We're not exposing the type info or using that function outside of this scope, so perhaps that's a sensibly practical option?
There was a problem hiding this comment.
The if typing.TYPE_CHECKING does actually help here. If trio (actually, trio-typing) is not installed, mypy is still happy because we use --ignore-missing-imports so it just ignores import trio and trio.Event().
There was a problem hiding this comment.
@tomchristie still want me to make that a typing.Any?
httpx/_dispatch/asgi.py
Outdated
|
|
||
| if response_complete: | ||
| if request_complete: | ||
| # Simulate blocking until the response is complete and then disconnect |
There was a problem hiding this comment.
I'd tweak this comment. We're not "simulating" blocking until the response is complete, we are blocking until the response is complete. :)
There was a problem hiding this comment.
Took out "streaming" and then found the comment a bit redundant 🤔
| pytest-trio | ||
| pytest-cov | ||
| trio | ||
| trio-typing |
There was a problem hiding this comment.
Do we need to add this dependency?
There was a problem hiding this comment.
For any trio types to be type-checked this must be installed (the only type being checked is trio.Event). This package also has a mypy plugin that can be enabled for extra functionality, which I did enable in encode/httpcore#69, but I didn't bother here.
|
Curious what the other @encode/maintainers think to this? Also, would we want it in a 0.13.dev2 release, along with encode/httpcore#81 perhaps? Seems decent @JayH5 - Are you able to confirm that it resolves Kludex/starlette#908? |
|
@tomchristie I can confirm that this fixes Kludex/starlette#908 for me 😄. Would love for this to be in a released version of httpx. Currently stuck with starlette <0.13.3 in some places until the issue is fixed. |
lovelydinosaur
changed the title
2 participants
Second attempt at fixing the problem described in Kludex/starlette#908. More discussion of this in #909.