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

Headers not being correctly validated as bytes. #276

Closed
carltongibson opened this issue Aug 7, 2019 · 7 comments · Fixed by #281
Closed

Headers not being correctly validated as bytes. #276

carltongibson opened this issue Aug 7, 2019 · 7 comments · Fixed by #281
Labels

Comments

@carltongibson
Copy link
Member

Refs django/channels#1331.

After e93643f header should be validated.

But the example provided in https://github.com/yuriiz/myproject/ just hangs.
Looking at it in the debugger the ValueError is raised but just disappears™ — it's doesn't correctly lead to an error response being returned. 🤔

@carltongibson
Copy link
Member Author

Hi @matthiask. Sorry to ping you out of the blue.

I don't if you have will/capacity to play with this issue but in django/channels#1246 you said:

I'm not convinced it's worth it because Daphne's error message is quite clear.

So you were getting the expected ValueError from check_headers_type().

Something's obviously changed since that's just hanging with the sample project (linked above). I thought maybe you might be interested to give it a run? (Please don't worry if not: I'll get to it myself in that case. 🙂)

@carltongibson
Copy link
Member Author

Looks related to django/channels#1254 (As per django/channels#1331 (comment))

@matthiask
Copy link

@carltongibson Thanks for the ping; I know there's an issue in the structure of the AsyncHttpConsumer (I think it currently does not support receiving messages from the channel layer at all, or at least it didn't in the version I added) but I'm not sure how to fix this. I have the suspicion that the current code does not return to await_many_dispatch as it should -- the http_request handler always terminates the HTTP connection and stops the consumer when more_body is falsy, but it shouldn't do that. I also suspect you already know this :-) but maybe it's helpful. Right now my workload at $dayjob does not allow me to put some time into channels (without painful reprioritizations), so if you want to solve this in the next days or weeks please don't wait for me.

@carltongibson
Copy link
Member Author

Ok. Thanks for the reply @matthiask. My goal here is a set of releases for September so if you do have time, awesome. If not no problem at all, its on the list. 👍

@matthiask
Copy link

Thank you!

I had an idea while doing the laundry -- let's see what Travis CI and reviewers say django/channels#1334

@carltongibson
Copy link
Member Author

Always the way. I’ll take a look later on. Thanks!

@carltongibson
Copy link
Member Author

Right, I have a draft PR in #281 for this one.

tl;dr I think the StopConsumer stuff is all fine (at least re this ticket). It's just that no response was ever sent to the client in this case, so it just sat there hanging forever...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants