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

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

Conversation

matthiask
Copy link
Contributor

Earlier self.handle always disconnected the HTTP client and stopped
the consumer when exiting. This meant that the consumer couldn't receive
any messages from the channel layer at all, making it impossible to send
e.g. messages from workers to connected HTTP clients via long polling or
server-sent events.

…` already

Earlier `self.handle` always disconnected the HTTP client and stopped
the consumer when exiting. This meant that the consumer couldn't receive
any messages from the channel layer at all, making it impossible to send
e.g. messages from workers to connected HTTP clients via long polling or
server-sent events.
@matthiask
Copy link
Contributor Author

There were reasons (tm) for the addition of the StopConsumer and self.disconnect lines, but I'm not sure I understand them after rereading the pulls. Maybe now we're back to the situation where a dying consumer does not disconnect the HTTP connection?

Also, it's a bit unexpected that tests pass. I mean it's nice obviously, but I also suspect that coverage is ... not very good in this particular area.

@carltongibson carltongibson self-assigned this Dec 2, 2019
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @matthiask. Slow follow-up, but I think you're on the right track here.

tl;dr We shouldn't be catching the exception. Rather we should let it bubble up to Daphne, which can then handle:

  • putting together a sensible response for the client.
  • Ensuring that the consumer gets shut down properly.

I need to work on the exact set of tests I want here, but we have a lead :)

Thanks for your input. (I'm not going to merge this but I'll leave it open whilst I'm working on it.)

@@ -43,6 +43,9 @@ def __init__(self, *args, **kwargs):
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

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.

@carltongibson carltongibson mentioned this pull request Feb 26, 2020
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?

Base automatically changed from master to main March 3, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants