Skip to content

Adding logic to allow the client to decide if the websocket frame mask should be used when sending messages. #999

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

QuinnDamerell
Copy link
Contributor

After debugging perf issues on lower-end devices like a Pi4 and Pi3, I found that the WebSocket lib was performing a masking operation over the entire buffer sent, as the websocket standard requires. Unfortunately, that masking operation is quite costly; I measured that it doubled the amount of time required to send a ~10kb buffer.

I looked into why the masking is done, and it was added due to the security issue discussed in this paper. The problem is that the paper was published in 2011 and deals with unencrypted web sockets. Since almost all internet traffic WebSockets now use SSL, there have been a few attempts to remove the masking from the protocol since it adds overhead and isn't needed anymore.

I added a simple optional flag to the send call that defaults to masking enabled (as the standard requires), but it allows the client to disable the mask if desired. As I said above, if the client knows the websocket is opened via SSL or it's only being used on a local LAN, the mask is not needed. As I also said, skipping the mask resulted in dropping the send time by half and reducing the CPU percentage required by 10% in my application.

From my testing, most modern web servers will accept client messages without masking, so this seems to be a helpful option for lower-end IOT devices.

If there are any questions, I would love to discuss them!

@engn33r
Copy link
Collaborator

engn33r commented Oct 15, 2024

I'll look again soon to collect any final comments before merging, but I wanted to say this looks good, thanks for the PR 👍

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.

2 participants