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

Ensure text message exists before handling on WebsocketConsumer #2097

Merged

Conversation

cacosandon
Copy link
Contributor

When using uvicorn or daphne, only one of text or bytes is sent.

But on hypercorn, both of them are sent, but with None value:
https://github.com/pgjones/hypercorn/blob/31639ec2f4d03aa920b95c84686163901224c6cf/src/hypercorn/protocol/ws_stream.py#L159

So if you just send bytes, text will be None and you won't be able to handle the message.

I just add an extra check.

@bigfootjon
Copy link
Collaborator

Hey @cacosandon, this looks reasonable to me. Can you add a regression test?

@cacosandon cacosandon force-pushed the fix/hypercorn/websockets-message-handler branch from e8a795e to 26fdd81 Compare September 3, 2024 03:48
@cacosandon
Copy link
Contributor Author

Sorry for the super late reaction @bigfootjon, I've added a regression test! Had to use send_input and send_output because WebsocketCommunicator has some safe checks that hypercorn is skipping.

@cacosandon cacosandon force-pushed the fix/hypercorn/websockets-message-handler branch from e539976 to 43a83c6 Compare September 3, 2024 17:48
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.

LGTM. Thanks @cacosandon

@bigfootjon any last thoughts?

@carltongibson
Copy link
Member

(Sorry, @cacosandon : can you fix the lint error? Thanks.)

Copy link
Collaborator

@bigfootjon bigfootjon left a comment

Choose a reason for hiding this comment

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

LGTM!

@bigfootjon bigfootjon merged commit 643d083 into django:main Sep 4, 2024
6 checks passed
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