-
-
Notifications
You must be signed in to change notification settings - Fork 800
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 improvements #1255
Open
MarioRial22
wants to merge
7
commits into
django:main
Choose a base branch
from
MarioRial22:master
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f69c918
AsyncHttpConsumer improvements, don't disconnect when handle is done,…
09cde3f
TEST WIP
22646ca
Test improvements
d815c5a
imports order
ad6fe5a
reformat file
753a024
Merge branch 'master' of github.com:django/channels
365b9f4
test streaming
jkarneges File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wander if it would be nicer to have a
exception_handler
method on theAsyncHttpConsumer
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)
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.
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.
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.
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 whyfinally
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....)
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 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 raisesStopConsumer
any exception that was raised is then dropped by the AsyncConsume parent classes__call__
method.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.
@carltongibson what do we think about:
set_exception_handler
orCancelledError
being used here? That/and/or having us create anexception_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?
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.
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.)