-
Notifications
You must be signed in to change notification settings - Fork 225
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
Add permessage-deflate
support
#328
Add permessage-deflate
support
#328
Conversation
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 did not check it rigorously, but it looks like a sane implementation, thanks for the amount of work that has been put into it! It also looks like it worked pretty well (based on the comments to the previous iteration of the PR).
It's a significant change, so I'll also additionally ask @agalakhov to approve it so that we can safely merge it!
Any review status on this? |
There have not been any comments, so I assume that everyone is okay with the changes, so let's give it a go! Thanks again for the large amount of work that has been put into that. |
I was fighting for quite a while trying to get this work and it didn't... until I finally realized that it was merged in yesterday! Long time in the making, glad to finally have it implemented. |
@daniel-abramov As I mentioned in the PR, this depends on hyperium/headers#88. |
Oh, yeah, thanks for reminding me, I've forgotten about that! Though maybe we could specify it in the README if we want to publish it as a separate feature to let people decide. Hopefully, the hyperium/headers#88 gets reviewed and merged. It seems like maintainers did not have time to address it recently or something like that. It seems like people really want to use permessage-deflate with WebSockets. |
You won't be able to publish
Yeah, it can be significant depending on your messages.
I've been using the original implementation (#235) in production for almost 2 years without any issues, but the new implementation in this PR is not battle tested. It should be equivalent and have better error handling. Anyway, I'm just surprised this was merged. Are you the only active maintainer? |
Yeah, that's unfortunate. I may need to move it to a branch (which essentially would defeat the whole purpose of merging I think). It felt like if we don't merge it, it would stagnate and eventually will get obsolete (conflicts with the master branch etc), but at the same time, I'd forgotten about the git dependencies.
That does not sound too bad.
Well, sort of. I'm not the only maintainer, but the only one who has been more or less active in the past years. UPD: Reverted the change, and moved the implementation to the branch until the dependency is updated. |
This reverts commit edb2377. See #328 (comment)
// "server_max_window_bits" extension parameter. | ||
// | ||
// However, but we need to fail the connection because we don't support it (condition 4). | ||
return Err(DeflateError::Negotiation( |
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.
This is incorrect. Server may include "server_max_window_bits" without the client asking for it for the explicit reason that it informs the window size for compression that the server uses. There is no harm for the client to ignore that value, and use a full sized window for decompression - and since our window is 15 bits by default now that it isn't configurable, any valid value from server is fine for the client.
The correct behaviour is to have the normal duplicate checking for this response header, but accept any valid value (or any value below the window size we use, but flate2 doesn't expose that so we will just need to assume it's 15 bits).
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.
Thanks for your comment.
@kazk The pull is merged, and the work is stashed in a branch and now there's no open pull for this... but added one review comment that has an impact on the interoperability of the client. |
This reverts commit edb2377. See snapview#328 (comment)
So it seems someone could rebase this PR on master, but putting the |
I'm working on integrating |
Supersedes #235.
permessage-deflate
is not enabled by default and requires adeflate
feature.client_max_window_bits
andserver_max_window_bits
are not supported yet because these requireflate2/zlib
which is not pure Rust.Sec-WebSocket-Extensions
header is parsed byheaders
SecWebsocketExtensions
hyperium/headers#88permessage-deflate
support tokio-tungstenite#251warp
integration: Add WebSocketpermessage-deflate
extension support seanmonstar/warp#1016I might need to adjust some code depending on hyperium/headers#88, but this should be ready for review.
Closes #2.