AsyncHttpConsumer improvements

[MarioRial22]

Hello!

This is my first contribution to the framework as a follow up to this issue: #1254 .
Please be kind when reviewing :)

I ran through a couple of problems using the current AsyncHttpConsumer:

• Any exception produced was absorbed, and couldn't see any traceback.
• Exceptions were not closing the HTTP response (like they do closing the socket on websocket consumers). When an exception bubbles up http_disconnect is not called, causing the consumer to remain open forever.
• After the handle method the consumer was stopped, causing issues like this: AsyncHttpConsumer regression and disconnect handling #1249

This is WIP proposed solution that addresses some of the issues:

• It provides a close method that allows the consumer to close the response from the server end.
• Doesn't stop the consumer after handle. IMO the consumer should only be stopped on http_disconnect

Probably some issues that I'm addressing here should be addressed at the server layer?
Some of the code TODO comments are informative and will remove them after code review.

I included in the PR the test case proposed on: #1250
Since this PR fixes that case as well.

[carltongibson]

Hi @MarioRial22. Can you add test cases showing the behaviour you're fixing here?

[auvipy]

could you please fix the lint errors to trigger the test matrix?

[MarioRial22]

Hey @auvipy fixed.

Could you guys take a look at the TODOs that I wrote on the AsyncHttpConsumer?
Essentially when we bubble up exceptions http_disconnect is not being called by the server, when I think it should.
I chose to patch it right there calling self.close. Which will get disconnect called since it will set more_body=False.

[MarioRial22]

I also included the test case suggested on: #1250
Since this PR fixes that case as well.

[carltongibson]

Hi @MarioRial22. Thanks for the work here. Let me get DRF v3.10 out the way and then I'll look at this (and other PRs here) and we'll get a new release out. 👍

[MarioRial22]

Sounds good @carltongibson !

[carltongibson]

Hi @MarioRial22. Just to let you know, I've swung round to this and am giving it a look. Targeting releases in Sept, so by then. 🙂 Thank you for your effort!

[hishnash]

I wander if it would be nicer to have a exception_handler method on the AsyncHttpConsumer that sends this 500 down the wire.

In some cases we need to format this 500 response a little differently (eg, if we got a Redis connection exception we might well want to push some retry after headers on the response)

[MarioRial22]

Interesting... if you want to format it differently you can always catch the exception in your class and handle it in your code.
Honestly I'm not familiar enough with the code base, I think when any exception bubbles up the http connection should be closed and the consumer stopped, but that doesn't happen for some reason. So this is the fix that I found.

[adamhooper]

btw ... be sure to re-raise asyncio.CancelledError. If you don't re-raise it, asyncio produces undefined behavior.

For that matter ... shouldn't every exception be re-raised? I'm a keen user of asyncio.EventLoop.set_exception_handler() -- it helps find bugs on production. (I imagine this is why finally was used in the first place.)

(To be clear: I don't know much about Channels' innards. I'm just worried because changing catch-all behavior seems like it'll have far-reaching effects....)

[hishnash]

I agree un-expected exceptions should not be swallowed so catching in self.handle and sending a response would not be ideal.

the reason for wanted to be able to manage the 500 response body is that for our server we want to return a JSON error bodys that is parsable. At the moment our solution for this is to put a middleware layer between the Consume and the top level Application that proxies all send messages however since this does not have access to the source exception it can't produce more than a very generic error.

@adamhooper The issue with the finally approach currently in master is that it replaces effectively swallows your exception, since finally raises StopConsumer any exception that was raised is then dropped by the AsyncConsume parent classes __call__ method.

[jheld]

@carltongibson what do we think about:set_exception_handler or CancelledError being used here? That/and/or having us create an exception_handler method to keep the flow more organized?

Are any of the pieces in this discussion required vs optional, or is the flow a bit beyond straightforward rule-logic?

[carltongibson]

My thought on this, when I looked at it, was that we're entering a loosing battle. (I catch an exception, try and call close, and end-up awaiting body — but hang-on! I was closing.)

Rather, for this kind of something I really wasn't expecting went wrong case, I think we should just let the exception bubble up to Daphne, where it will be handled with a plain response (and perhaps we can add some logging to help the related issues around this space...).

Then, assuming that all looks good, we need to make sure Daphne sends the appropriate http_disconnect to stop the consumer. (Which looks like it's not happening.)

(Of course, this isn't fresh in my mind so I may need to fire up the debugger again to get back on top of it.)

[carltongibson]

Hi @MarioRial22. Thanks for this. Sorry for the slow uptake. It just takes a while to get the free space.

tl;dr You're right, 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. (Rather than doing that here.)
• Ensuring that the consumer gets shut down properly, via http_disconnect.

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 as-is but I'll leave it open whilst I'm working on it.)

[jheld]

is there a way to test that the connection has closed? and if so, is there any use in doing that? I'm requesting this because there is logic change here that is around the disconnect flow, so is it worthwhile to check that we really did disconnect?

I realize this is probably for SSE (server sent events), but just want to make sure that we aren't forgetting to check for the close, if we can/should.

[jheld]

here too do/should we check for connection close?

[hishnash]

Is there a chance of any progress on this PR (we are currently depending on this fork in our systems due to the improved error handling). If there is anything I can do to help get this landed I have time to work on this, also make changes to Daphne that it is the one producing the 500.

[MarioRial22]

Hey @hishnash I'm also depending on this fork for a pretty big project. And have been relying on this for over a year now.
I'll try to find sometime next week to work on this!

[carltongibson]

There's two cases here:

1. A, let's say, unexpected error in your consumer, raised after the response is started. Here I think it should be Daphne's job to clear up, log appropriately, and make sure the response is finished cleanly. (If you error early Daphne will already send a 500.)
2. A, let's say, expected error in your consumer, which you might catch, and then have the response finish cleanly.

In case 1 I think we shouldn't catch the exception, and adjust Daphne to make sure it handles such a case. We're not currently doing that. (Any input there welcome. 🙂)

In case 2 I'm not sure what to say. If you know your consumer might error out, be prepared to catch that yourself. (Or so the thought goes.) I'm not sure we should complicate the consumers to handle such cases. ("If you know your consumer might error out, be prepared to catch that yourself.")

[hishnash]

@carltongibson with respect too Daphne returning 500 to users on exception, is that not already handled by https://github.com/django/daphne/blob/61c8633c5d834e24c8d5473df522efa849a60e8d/daphne/server.py#L273-L291

(all we need to do is ensure that channels raises the error, rather than swallow it)

this PR also containes changes to how channels tracks when a http response is complete do you feel these changes are good to keep?

for handling custom exceptions at a consumer level I think users can do that in the self.handle method on their subclasses.

(or in a middleware that intercepts them on the way up the stack).