Skip to content
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

Merged
merged 6 commits into from
May 12, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions httpx/_dispatch/asgi.py
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
Copy link
Member

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?...

try:
    import trio
    Event = typing.Union[asyncio.Event, trio.Event]
except ImportError:
    Event = asyncio.Event

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.

Copy link
Member Author

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:

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

Copy link
Member

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?

Copy link
Member Author

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. 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().

Copy link
Member Author

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?


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):
"""
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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. :)

Copy link
Member Author

Choose a reason for hiding this comment

The 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}

Expand All @@ -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

Expand Down
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pytest-asyncio
pytest-trio
pytest-cov
trio
trio-typing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add this dependency?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 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.

trustme
uvicorn
seed-isort-config
Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def get_packages(package):
"idna==2.*",
"rfc3986>=1.3,<2",
"httpcore>=0.8.1",
"sniffio",
],
classifiers=[
"Development Status :: 4 - Beta",
Expand Down
3 changes: 1 addition & 2 deletions tests/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ async def test_asgi_exc_after_response():
await client.get("http://www.example.org/")


@pytest.mark.asyncio
async def test_asgi_disconnect_after_response_complete():
async def test_asgi_disconnect_after_response_complete(async_environment):
disconnect = False

async def read_body(scope, receive, send):
Expand Down