tungstenite-rs icon indicating copy to clipboard operation
tungstenite-rs copied to clipboard

Add `permessage-deflate` support

Open kazk opened this issue 3 years ago • 9 comments

  • 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: https://github.com/snapview/tokio-tungstenite/pull/190
    • warp: https://github.com/seanmonstar/warp/compare/master...kazk:permessage-deflate
    • Example using warp with compression: https://github.com/qualified/lsp-ws-proxy/commit/89fce502cf792cfa89e814865bbe170666499c0d

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?

  • [x] deflate feature
  • [x] 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

kazk avatar Sep 13 '21 19:09 kazk

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

kazk avatar Sep 14 '21 17:09 kazk

@kazk Any chance this is still being worked on?

jenanwise avatar Feb 27 '22 04:02 jenanwise

I was planning to continue after getting https://github.com/hyperium/headers/pull/88 merged (see https://github.com/snapview/tungstenite-rs/pull/235#discussion_r742305530), 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 https://github.com/hyperium/headers/pull/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 avatar Feb 28 '22 22:02 kazk

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.

kazk avatar Mar 05 '22 09:03 kazk

Improve parsing Sec-WebSocket-Extensions. I did in https://github.com/hyperium/headers/pull/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?

daniel-abramov avatar Mar 08 '22 18:03 daniel-abramov

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 avatar Mar 08 '22 21:03 kazk

I've updated to use headers. https://github.com/kazk/tungstenite-rs/commit/2e404a447c94eb05f3ee4059b7094bad06e20d1f

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

kazk avatar Mar 14 '22 07:03 kazk

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

kazk avatar Mar 15 '22 06:03 kazk

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

daniel-abramov avatar Mar 17 '22 13:03 daniel-abramov