-
Notifications
You must be signed in to change notification settings - Fork 216
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
Permessage-deflate extension #144
Conversation
Permessage-deflate
Merge base
Outbound message chunking
TX message chunking
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.
First of all: thanks for the PR and the changes that you made!
I left some comments, which I think must be addressed before we can merge them. I also have not yet checked deflate.rs
implementation.
Also I have one general question: do we really need an additional type parameter in all places? Maybe we can avoid it? Having a generic trait for extensions is fine, but can't we use many extensions and is a Config
a right place to choose the extension? I'm not sure at the moment. It seems like the current implementation assumes that encoder
is just a concrete implementation of permessage-deflate, rather than a generic extension or list of extensions.
@agalakhov could you please check the comments that I left to this PR and check if I missed something? This is quite a huge PR and I would check it carefully before we can merge it. But I think there is some work that has to be done.
optional = true | ||
version = "1.0" | ||
default-features = false | ||
features = ["zlib"] |
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, do we want to rely on zlib
? @agalakhov, what do you think?
@@ -58,7 +58,7 @@ Features | |||
Tungstenite provides a complete implementation of the WebSocket specification. | |||
TLS is supported on all platforms using native-tls. | |||
|
|||
There is no support for permessage-deflate at the moment. It's planned. | |||
Permessage-deflate. |
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.
Perhaps we can remove this line now ;)
if hdr.rsv1 || hdr.rsv2 || hdr.rsv3 { | ||
return Err(Error::Protocol("Reserved bits are non-zero".into())); | ||
} | ||
} |
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.
Does it have any adverse effects that we postpone this check?
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.
If the deflate extension is disabled, then it delegates the work to the uncompressed extension and does not mutate the frame in any way.
/// Create a WebSocket context that manages a post-handshake stream. | ||
pub fn new(role: Role, config: Option<WebSocketConfig>) -> Self { | ||
pub fn new(role: Role, config: Option<WebSocketConfig<Ext>>) -> Self { | ||
let config = config.unwrap_or_else(Default::default); |
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.
Out of curiosity: why did you decide to move out the config creation line?
src/extensions/deflate.rs
Outdated
@@ -0,0 +1,922 @@ | |||
//! Permessage-deflate extension |
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.
Did you write this file manually? Or is it a modified copy-paste from deflate
? I'm asking, because it's quite a huge file and I and @agalakhov will have to validate it carefully as it contains loop
s, unwrap
s and some copy-pasting.
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.
It's an adaptation of WS-RS's deflate extension to use ZLib instead of libc, and to work with a growable fragment buffer.
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.
Got it. I think I or @agalakhov will have to take a close look at it. At the first glance, it seems like it contains some copy-paste that we would like to avoid :)
Hi @application-developer-DA, thank you for the review. Regarding the additional type parameter, the thought behind this was that it allowed for users to implement their own extensions and provide them. An alternative approach to this would be to have permessage-deflate configurable in the Regarding your error handling/into comments, the |
@SirCipher , yes, I think having it inside a config makes sense. It seems like it would simplify many things. What do you think? |
…o deflate-merge � Conflicts: � src/protocol/mod.rs
# Conflicts: # examples/autobahn-client.rs # examples/autobahn-server.rs # src/client.rs # src/handshake/client.rs # src/handshake/server.rs # src/lib.rs # src/protocol/frame/frame.rs # src/protocol/frame/mod.rs # src/protocol/mod.rs # tests/connection_reset.rs
What's the status of this PR? |
@novacrazy the PR does not pass all of our CI/CD checks and has some conflicts, thus it can't be merged. I'm going to look at the content one more time to see if the rest is fine. |
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 doing that great progress, but unfortunately there are several reasons why we can't merge it right now:
- The PR does not pass our current CI/CD pipeline.
- It looks like part of this PR should be done as a separate library, i.e. I think that the whole deflate/compression/uncompression logic here should not be part of
tungstenite-rs
as it could be generalized and does not have anything to do with WebSockets. This would make the implementation much more simple and concise. I'm afraid that if we merge it "as is" (even if (1) is fixed), we would introduce some technical debt to the library which we would like to avoid if possible, unless I'm missing on something.
Due to time constraints on my end and the additional changes required to make the requested changes, I won't be able to finish this PR in the near future. |
This PR adds support for permessage-deflate for both inbound and outbound messages through the use of WebSocket extensions.
I've also added this support to the tokio-tungstenite crate in a separate PR.
This is feature-gated through the
deflate
flag and uses the Flate2 crate for inflating and deflating messages.I've updated the client and server Autobahn results and all unit tests for both pass.