Add `permessage-deflate` support #235

kazk · 2021-09-13T19:11:06Z

Adds de/compression logic on top of the existing flow, requiring minimal changes. Shouldn't have any major breaking changes.
client_max_window_bits/server_max_window_bits are not supported at the moment because that requires flate2/zlib feature. We can enable them with a feature flag later if there's a demand.
Passes autobahn except for 13.3 to 13.6, that requires supporting max window bits
Can be used from crates like warp and axum.
- tokio-tungstenite: Add permessage-deflate support tokio-tungstenite#190
- warp: seanmonstar/warp@master...kazk:permessage-deflate
- Example using warp with compression: qualified/lsp-ws-proxy@89fce50

This works for my use case, but I'd like some feedback from maintainers and users before polishing this to be merged.

Does it make sense to think about supporting more PMCEs (e.g., permessage-bzip2 and permessage-snappy)? These are not standardized as far as I know, and browsers don't support them.
~~I think including flate2 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 for flate2/zlib and support max window bits.~~
Should the default WebSocketConfig enable compression support?

deflate feature
Parsing Sec-WebSocket-Extensions more strictly with headers (https://github.com/kazk/tungstenite-rs/tree/permessage-deflate-rebased)
Error on invalid negotiation response
Error handling
deflate-zlib feature for max window bits

Closes #2

daniel-abramov

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.

daniel-abramov · 2021-09-14T15:16:52Z

autobahn/expected-results.json

@@ -3,56 +3,56 @@
      "1.1.1": {
         "behavior": "OK",
         "behaviorClose": "OK",
-         "duration": 1,
+         "duration": 2,


The duration value was derived empirically, right? 🙂

I just ran scripts/autobahn-client.sh and scripts/autobahn-server.sh locally. Some durations will change every time.

Cargo.toml

src/error.rs

daniel-abramov · 2021-09-14T15:28:25Z

src/extensions/compression/deflate.rs

+impl Default for DeflateConfig {
+    fn default() -> Self {
+        Self {
+            compression: Compression::default(),
+            server_no_context_takeover: false,
+            client_no_context_takeover: false,
+        }
+    }
+}


Won't it work to add Default in #[derive(..., Default)] to the DeflateConfig?

Yeah, that works at the moment, but bools that defaults to true like accept_server_no_context_takeover seems like a common option, and we should probably support.

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?)

daniel-abramov · 2021-09-14T16:18:27Z

src/extensions/mod.rs

+    }
+}
+
+// NOTE This doesn't support quoted values


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.

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.

Sure, usage of headers is fine!

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.

src/extensions/compression/deflate.rs

daniel-abramov · 2021-09-14T17:05:51Z

src/extensions/compression/deflate.rs

+                CLIENT_MAX_WINDOW_BITS => {}
+
+                _ => {
+                    //


An error? :)

src/extensions/compression/deflate.rs

src/protocol/mod.rs

daniel-abramov · 2021-09-14T17:18:23Z

src/protocol/mod.rs

+            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;
+                }
            }


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.

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.

kazk · 2021-09-14T17:55:23Z

@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.

jenanwise · 2022-02-27T04:17:18Z

@kazk Any chance this is still being worked on?

kazk · 2022-02-28T22:32:08Z

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 warp in production.

Will try to take a look this week after work.

I think the following needs to be done to get this merged:

Improve parsing Sec-WebSocket-Extensions. I did in Add SecWebsocketExtensions hyperium/headers#88, but we can include that here for now if necessary.
Validate negotiation response
Error handling in general

Nice to have:

deflate-zlib feature to support max window bits

kazk · 2022-03-05T09:36:05Z

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.

daniel-abramov · 2022-03-08T18:07:36Z

Improve parsing Sec-WebSocket-Extensions. I did in hyperium/headers#88, but we can include that here for now if necessary.

NB: Should we probably wait a bit for a merge of the PR in headers so that we can simplify the implementation in the tungstenite?

kazk · 2022-03-08T21:49:29Z

Yeah, I'd like to keep that in headers.

I'll update this PR to use headers (using my fork for now), and make sure it's useful for tungstenite. After making any necessary changes, I'll ask them to review that PR again and try to get that merged.

kazk · 2022-03-14T07:20:24Z

I've updated to use headers. kazk@2e404a4

I'll try to spend some time during the week to finish it and open a new PR.

kazk · 2022-03-15T06:12:48Z

@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. ~~Is this a regression? I can open a separate PR to update if it just needs to be updated.~~

These are tests for Clean close with normal or echoed code. I guess tungstenite used to close with 1000, but now closes with echoed code. #246?

Opened #271

diff expected master:

     "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"
     }

daniel-abramov · 2022-03-17T13:46:49Z

@kazk thanks for the investigation! The corresponding PRs have been merged.

kazk · 2023-01-07T01:27:45Z

Superseded by #328. If anyone is using this branch, I'm not deleting it because I also have some projects depending on it.

Add permessage-deflate support

54acf30

This was referenced Sep 13, 2021

Support for permessage-deflate [✅/awaiting_dependency_merge] #2

Open

WebSocket permessage-deflate extension support seanmonstar/warp#770

Open

Add permessage-deflate support snapview/tokio-tungstenite#190

Closed

daniel-abramov reviewed Sep 14, 2021

View reviewed changes

kazk added 2 commits September 14, 2021 12:15

Capitalize error Display

147593f

Make is_compressed immutable

fdb82ed

kazk force-pushed the permessage-deflate branch from fa45b35 to fdb82ed Compare September 14, 2021 19:37

kazk added 7 commits September 14, 2021 13:04

Support multiple Sec-WebSocket-Extensions request headers

c390986

Extract compression to a separate method

f6e0b80

Rename to generate_offer

73ef209

Remove parse_header and Param

40eb923

Add extension negotiation methods to WebSocketConfig

8dc8a41

Clean up accept_offer

78322fe

Make DeflateContext private and add Extensions container

c62eccc

kazk force-pushed the permessage-deflate branch from 0cb98c2 to 8b44d12 Compare September 15, 2021 07:49

Add deflate feature

734a0b9

kazk force-pushed the permessage-deflate branch from 8b44d12 to 734a0b9 Compare September 15, 2021 08:02

tibkiss mentioned this pull request Oct 10, 2021

Support for compression vi/websocat#47

Open

PooyaEimandar mentioned this pull request Nov 5, 2021

Implement WebSocket WolfSource/WolfEngine#145

Closed

kazk mentioned this pull request Apr 25, 2022

[Doubt] Using as library qualified/lsp-ws-proxy#33

Open

kazk mentioned this pull request Oct 17, 2022

Add SecWebsocketExtensions hyperium/headers#88

Draft

kazk mentioned this pull request Jan 7, 2023

Add permessage-deflate support #328

Merged

1 task

kazk closed this Jan 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `permessage-deflate` support #235

Add `permessage-deflate` support #235

kazk commented Sep 13, 2021 •

edited

daniel-abramov left a comment

daniel-abramov Sep 14, 2021

kazk Sep 14, 2021 •

edited

daniel-abramov Sep 14, 2021

kazk Sep 14, 2021 •

edited

daniel-abramov Oct 14, 2021

daniel-abramov Sep 14, 2021

kazk Sep 14, 2021

daniel-abramov Nov 3, 2021

kazk Nov 3, 2021

daniel-abramov Sep 14, 2021

daniel-abramov Sep 14, 2021

kazk Sep 14, 2021 •

edited

kazk commented Sep 14, 2021

jenanwise commented Feb 27, 2022

kazk commented Feb 28, 2022

kazk commented Mar 5, 2022

daniel-abramov commented Mar 8, 2022

kazk commented Mar 8, 2022

kazk commented Mar 14, 2022

kazk commented Mar 15, 2022 •

edited

daniel-abramov commented Mar 17, 2022

kazk commented Jan 7, 2023

Add permessage-deflate support #235

Add permessage-deflate support #235

Conversation

kazk commented Sep 13, 2021 • edited

daniel-abramov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazk Sep 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazk Sep 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazk Sep 14, 2021 • edited

Choose a reason for hiding this comment

kazk commented Sep 14, 2021

jenanwise commented Feb 27, 2022

kazk commented Feb 28, 2022

kazk commented Mar 5, 2022

daniel-abramov commented Mar 8, 2022

kazk commented Mar 8, 2022

kazk commented Mar 14, 2022

kazk commented Mar 15, 2022 • edited

daniel-abramov commented Mar 17, 2022

kazk commented Jan 7, 2023

Add `permessage-deflate` support #235

Add `permessage-deflate` support #235

kazk commented Sep 13, 2021 •

edited

kazk Sep 14, 2021 •

edited

kazk Sep 14, 2021 •

edited

kazk Sep 14, 2021 •

edited

kazk commented Mar 15, 2022 •

edited