-
Notifications
You must be signed in to change notification settings - Fork 218
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
#235
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.
Thanks for the change, that's quite a big addition 🙂
I tried to go through the code and also touch the questions that you asked in the PR description. Generally the implementation looks good and I really appreciate the effort! There are a couple of questions to answer / decisions to make that we need to decide upon though, I tried to comment on them close to the context in which they are introduced.
@@ -3,56 +3,56 @@ | |||
"1.1.1": { | |||
"behavior": "OK", | |||
"behaviorClose": "OK", | |||
"duration": 1, | |||
"duration": 2, |
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.
The duration
value was derived empirically, right? 🙂
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 just ran scripts/autobahn-client.sh
and scripts/autobahn-server.sh
locally. Some durations will change every time.
impl Default for DeflateConfig { | ||
fn default() -> Self { | ||
Self { | ||
compression: Compression::default(), | ||
server_no_context_takeover: false, | ||
client_no_context_takeover: false, | ||
} | ||
} | ||
} |
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.
Won't it work to add Default
in #[derive(..., Default)]
to the DeflateConfig
?
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.
Yeah, that works at the moment, but bool
s that defaults to true
like accept_server_no_context_takeover
seems like a common option, and we should probably support.
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.
Ah, ok I see. Did I get you right, you have in mind to make the defaults for things like accept_server_no_context_takeover
to be true
, that's why you decided to write the implementation of Default
by hand instead of deriving it? (i.e. so that once the PR is in the "final" state, true
values would be there as defaults?)
src/extensions/mod.rs
Outdated
} | ||
} | ||
|
||
// NOTE This doesn't support quoted values |
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.
Must we not support it if we want to complain with the RFC? (i.e. at least produce a sane code that does not fail when someone supplies a string with the quoted value)
It looks like the quoted values are part of the ABNF for the Sec-WebSocket-Extensions
header.
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.
Yeah, I should've been more clear that this PR is not ready (draft), and/or left TODO
comments instead. I mostly wanted feedback on the questions I had and overall approach, then work with the maintainers to discuss what's best for tungstenite
.
For headers, are you open to using https://github.com/hyperium/headers ? I'm working on adding SecWebsocketExtensions
there that provides an iterator on offers, so tungstenite
doesn't need to do this.
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.
Sure, usage of headers
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.
I opened hyperium/headers#88. Integrating with crates like warp
and axum
should be easy with that. But I think I need to update to hide more details for it to be merged.
CLIENT_MAX_WINDOW_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.
An error? :)
src/protocol/mod.rs
Outdated
let mut is_compressed = false; | ||
{ | ||
let hdr = frame.header(); | ||
if hdr.rsv1 || hdr.rsv2 || hdr.rsv3 { | ||
if (hdr.rsv1 && self.pmce.is_none()) || hdr.rsv2 || hdr.rsv3 { | ||
return Err(Error::Protocol(ProtocolError::NonZeroReservedBits)); | ||
} | ||
|
||
if hdr.rsv1 && self.pmce.is_some() { | ||
is_compressed = true; | ||
} | ||
} |
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 think we could use something like:
let is_compressed = {
... <that block of code> ...
}
So that we don't need a mut
.
But a general question: what if we have more options? E.g. apart from compressed
some other extensions? In this case we'll probably need to make the implementation a bit more generic and abstract, so that we don't clutter the code. Related to the previous comment.
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.
Made it immutable. We can also just use rsv1
instead of is_compressed
if you prefer.
If we have more extensions, we'll need to first check for any conflicts in their use of rsv
bits during handshake. Then something like this:
let (is_compressed, use_e2, use_e3) = self.validate_header(frame.header())?;
// or
let (is_compressed, use_e2, use_e3) = {
let hdr = frame.header();
if (hdr.rsv1 && self.pmce.is_none()) || (hdr.rsv2 && self.ext2.is_none()) || (hdr.rsv3 && self.ext3.is_none()) {
return Err(Error::Protocol(ProtocolError::NonZeroReservedBits));
}
(hdr.rsv1, hdr.rsv2, hdr.rsv3)
};
I don't know if an extension is allowed to use more than one bit.
@application-developer-DA Thanks for reviewing! I'll start answering them soon. I opened this PR as a draft since it's not done (especially the client side like error handling, as you noticed), but wanted early feedback. |
fa45b35
to
fdb82ed
Compare
0cb98c2
to
8b44d12
Compare
8b44d12
to
734a0b9
Compare
@kazk Any chance this is still being worked on? |
I was planning to continue after getting hyperium/headers#88 merged (see #235 (comment)), but forgot. I've been using this branch with a patched Will try to take a look this week after work. I think the following needs to be done to get this merged:
Nice to have:
|
For now, I've rebased and cleaned up the commits. https://github.com/kazk/tungstenite-rs/tree/permessage-deflate-rebased I might have some time over the weekend to continue. |
NB: Should we probably wait a bit for a merge of the PR in |
Yeah, I'd like to keep that in I'll update this PR to use |
I've updated to use I'll try to spend some time during the week to finish it and open a new PR. |
@application-developer-DA The autobahn results don't match the expected in https://github.com/snapview/tungstenite-rs/tree/master/ for both server and client. These are tests for Opened #271
"7.7.10": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 3000,
"reportfile": "tungstenite_case_7_7_10.json"
},
"7.7.11": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 3999,
"reportfile": "tungstenite_case_7_7_11.json"
},
"7.7.12": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 4000,
"reportfile": "tungstenite_case_7_7_12.json"
},
"7.7.13": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 4999,
"reportfile": "tungstenite_case_7_7_13.json"
},
"7.7.2": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1001,
"reportfile": "tungstenite_case_7_7_2.json"
},
"7.7.3": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1002,
"reportfile": "tungstenite_case_7_7_3.json"
},
"7.7.4": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1003,
"reportfile": "tungstenite_case_7_7_4.json"
},
"7.7.5": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1007,
"reportfile": "tungstenite_case_7_7_5.json"
},
"7.7.6": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1008,
"reportfile": "tungstenite_case_7_7_6.json"
},
"7.7.7": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1009,
"reportfile": "tungstenite_case_7_7_7.json"
},
"7.7.8": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1010,
"reportfile": "tungstenite_case_7_7_8.json"
},
"7.7.9": {
"behavior": "OK",
"behaviorClose": "OK",
- "remoteCloseCode": 1000,
+ "remoteCloseCode": 1011,
"reportfile": "tungstenite_case_7_7_9.json"
} |
@kazk thanks for the investigation! The corresponding PRs have been merged. |
Superseded by #328. If anyone is using this branch, I'm not deleting it because I also have some projects depending on it. |
client_max_window_bits
/server_max_window_bits
are not supported at the moment because that requiresflate2/zlib
feature. We can enable them with a feature flag later if there's a demand.warp
andaxum
.tokio-tungstenite
: Addpermessage-deflate
support tokio-tungstenite#190warp
: seanmonstar/warp@master...kazk:permessage-deflatewarp
with compression: qualified/lsp-ws-proxy@89fce50This works for my use case, but I'd like some feedback from maintainers and users before polishing this to be merged.
permessage-bzip2
andpermessage-snappy
)? These are not standardized as far as I know, and browsers don't support them.I think includingflate2
without feature flag is fine (not large, defaults to pure Rust). What do you think? If there's a demand, we can add a flag forflate2/zlib
and support max window bits.WebSocketConfig
enable compression support?deflate
featureSec-WebSocket-Extensions
more strictly withheaders
(https://github.com/kazk/tungstenite-rs/tree/permessage-deflate-rebased)deflate-zlib
feature for max window bitsCloses #2