-
-
Notifications
You must be signed in to change notification settings - Fork 833
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
ASGI: Wait for response to complete before sending disconnect message #919
Changes from 4 commits
1cec8df
620bc0c
bf6f194
45f2466
6365262
1979edb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,28 @@ | ||
import typing | ||
from typing import Callable, Dict, List, Optional, Tuple | ||
|
||
import httpcore | ||
import sniffio | ||
|
||
from .._content_streams import ByteStream | ||
|
||
if typing.TYPE_CHECKING: # pragma: no cover | ||
import asyncio | ||
import trio | ||
|
||
Event = typing.Union[asyncio.Event, trio.Event] | ||
|
||
|
||
def create_event() -> "Event": | ||
if sniffio.current_async_library() == "trio": | ||
import trio | ||
|
||
return trio.Event() | ||
else: | ||
import asyncio | ||
|
||
return asyncio.Event() | ||
|
||
|
||
class ASGIDispatch(httpcore.AsyncHTTPTransport): | ||
""" | ||
|
@@ -76,23 +95,27 @@ async def request( | |
status_code = None | ||
response_headers = None | ||
body_parts = [] | ||
request_complete = False | ||
response_started = False | ||
response_complete = False | ||
response_complete = create_event() | ||
|
||
headers = [] if headers is None else headers | ||
stream = ByteStream(b"") if stream is None else stream | ||
|
||
request_body_chunks = stream.__aiter__() | ||
|
||
async def receive() -> dict: | ||
nonlocal response_complete | ||
nonlocal request_complete, response_complete | ||
|
||
if response_complete: | ||
if request_complete: | ||
# Simulate blocking until the response is complete and then disconnect | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took out "streaming" and then found the comment a bit redundant 🤔 |
||
await response_complete.wait() | ||
return {"type": "http.disconnect"} | ||
|
||
try: | ||
body = await request_body_chunks.__anext__() | ||
except StopAsyncIteration: | ||
request_complete = True | ||
return {"type": "http.request", "body": b"", "more_body": False} | ||
return {"type": "http.request", "body": body, "more_body": True} | ||
|
||
|
@@ -108,23 +131,23 @@ async def send(message: dict) -> None: | |
response_started = True | ||
|
||
elif message["type"] == "http.response.body": | ||
assert not response_complete | ||
assert not response_complete.is_set() | ||
body = message.get("body", b"") | ||
more_body = message.get("more_body", False) | ||
|
||
if body and method != b"HEAD": | ||
body_parts.append(body) | ||
|
||
if not more_body: | ||
response_complete = True | ||
response_complete.set() | ||
|
||
try: | ||
await self.app(scope, receive, send) | ||
except Exception: | ||
if self.raise_app_exceptions or not response_complete: | ||
raise | ||
|
||
assert response_complete | ||
assert response_complete.is_set() | ||
assert status_code is not None | ||
assert response_headers is not None | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ pytest-asyncio | |
pytest-trio | ||
pytest-cov | ||
trio | ||
trio-typing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to add this dependency? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For any trio types to be type-checked this must be installed (the only type being checked is |
||
trustme | ||
uvicorn | ||
seed-isort-config | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so
trio
might not be installed.Perhaps we should be doing something like this?...
Also 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried what you're suggesting but unfortunately run into an error like:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
if typing.TYPE_CHECKING
does actually help here. Iftrio
(actually,trio-typing
) is not installed, mypy is still happy because we use--ignore-missing-imports
so it just ignoresimport trio
andtrio.Event()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomchristie still want me to make that a
typing.Any
?