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.

Sign up for GitHub

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

AsyncHttpConsumer: Do not stop the consumer when exiting self.handle already #1334

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

AsyncHttpConsumer: Do not stop the consumer when exiting self.handle already #1334

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 4 additions & 5 deletions channels/generic/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ async def send_body(self, body, *, more_body=False):
await self.send(
{"type": "http.response.body", "body": body, "more_body": more_body}
)
if not more_body:
await self.disconnect()
raise StopConsumer()
Copy link
Member

Choose a reason for hiding this comment

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

This should be Daphne's job. (Via http_disconnect)

Copy link
Contributor

@jheld jheld Feb 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@carltongibson in this case, for checking if the body is fully sent/empty, is the expectation/correctness that await self.disconnect() is the right move? Or is the disconnect & StopConsumer the wrong flow?

I'm curious for how we signal up to the event loop manager (e.g. daphne, uvicorn) that we're ready for the disconnect to take plase. I can see why he considered doing the raise but unsure what the solution should be.

(At this time, due to lack of experience in the lower layer of this code, the disconnect sort of seems right, but yes, definitely happy to hear what should happen).

Copy link
Member

Choose a reason for hiding this comment

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

Hi @jheld.

I'm curious for how we signal up to the event loop manager (e.g. daphne, uvicorn) that we're ready for the disconnect to take plase.

My thought was that Daphne already knows that the response has finished at this point:

https://github.com/django/daphne/blob/59b57a9f4b7a419f1ab3ecb792ca802671045075/daphne/http_protocol.py#L256-L258

I think :) -- it was a couple of months ago when I looked at this.

Copy link
Member

Choose a reason for hiding this comment

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

@jheld See the discussion on #1255: I think that´s more telling that.


async def send_response(self, status, body, **kwargs):
"""
Expand Down Expand Up @@ -78,11 +81,7 @@ async def http_request(self, message):
if "body" in message:
self.body.append(message["body"])
if not message.get("more_body"):
try:
await self.handle(b"".join(self.body))
finally:
await self.disconnect()
raise StopConsumer()
await self.handle(b"".join(self.body))
Copy link
Contributor

Choose a reason for hiding this comment

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

So simplifying just to await self.handle(...) the intention is that no matter what's going on with message (additional bytes in it or not), we want/need to let the runner (daphne etc) deal with/control the rest of the flow, naturally.

@carltongibson so this specific change is correct/ready?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @jheld. Yes, I think so.

The reason I haven't pushed on here is that I think we need to make sure Daphne is able to correctly handle the case where we started the response and then error-ed after some time (thinking SSE or such). Currently it'll send an entire response, where we need it to just finish up the current one. (Q: what would uvicorn do in that case?)

Tests for all that.

Also I think we can document: "if your consumer is liable to raise an exception, you should probably add logic to handle that, otherwise you're falling back to default handling at the ASGI server level". (Or such)

(I think the other PRs here are essentially trying to build that logic into the generic consumers...)

Make sense?


async def http_disconnect(self, message):
"""
Expand Down