search

vapor / websocket-kit

WebSocket client library built on SwiftNIO
https://docs.vapor.codes/4.0/advanced/websockets/
MIT License
272 stars 79 forks source link

Adding Per Message Compression Extensions from RFC-7692. #142

Closed jhoughjr closed 1 year ago

jhoughjr commented 1 year ago

My first PR is PMCE support for WebSocket server and client.

There is an accompanying PR going to Vapor to support the changes to the WebSocketUpgrader to enable conditional use of PMCE.

I've tried to comment everything as descriptive as possible but without knowledge of the RFC, the code may not make obvious sense.

The changes are mostly encapsulated in the new PMCE class and its members. Websocket's main changes are an optional PMCE which is Sendable and some optional delegation to it if configured and enabled for use in the send and handle methods.

WebSocketClient.Configuration also has an optional PMCE so the client can configure its side. The public API is mainly through converting DeflateConfigs <> HTTPHeaders. ie if you want to serve pace web sockets, create the desire DeflateConfig and grab its headers. Pass them out of shouldUpgrade. As a client, do the same and pass them into the connect override with the ClientConfiguration.

I have some test apps and examples in another repo and am contiuning to write documentation. I have tested client against a node.js , a go and a vapor/websocketkit (this) implementation of PMCE servers. I have tested the vapor/websocket-kit server against the vapor/websocket-kit client, and a web browser client, all of which works, but I'm sure its not perfect yet.

I believe MahdiBM has some experience with this issue as well as adamfowler and may be useful ion review. I can answer any questions in discord.

koraykoska commented 1 year ago

@0xTim You are not serious, are you? As much as I like Vapor and use it every day, websocket-kit has had multiple breaking changes in the last years. Most recently, one of the latest releases (with the Sendable stuff) broke literally every single app I had using websocket-kit directly, within a minor version. And I can prove it if you need to.

There is literally no QA going on in any of the Vapor repositories. I trust @adam-fowler with compress-nio in a pre-1.0 version 10000% more than any of the Vapor repositories, especially websocket-kit, to not introduce any breaking changes.

Copying over code will very surely introduce another weak point in a barely maintained repository. I and surely many others would much rather depend on a maintained version of code doing a crucial part of what's being implemented in this PR.

If you really insist, why not simply version-lock it? Or reduce to bug-fix releases only? And then manually update the dependency if it makes (probably non-breaking) minor bumps.

This PR is very good and feature-complete. It even takes care of edge cases covered in the RFC. It introduces a long-awaited feature that is a crucial part of any real websocket library in any language. Vapor being the de-facto library used for Swift backends, you are the only one of any languages I know without websocket compression support. I would put priority on getting this merged ASAP.

A thorough code review will follow.

Joannis commented 1 year ago

Also want to mention this exists

image
Joannis commented 1 year ago

As per this comment you could consider integration testing with other implementations

codecov-commenter commented 1 year ago

Codecov Report

Merging #142 (a6ef521) into main (ffe0055) will decrease coverage by 19.74%. The diff coverage is 41.09%.

:exclamation: Current head a6ef521 differs from pull request most recent head 756216d. Consider uploading reports for the commit 756216d to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #142 +/- ## =========================================== - Coverage 82.85% 63.11% -19.74% =========================================== Files 6 7 +1 Lines 659 1231 +572 =========================================== + Hits 546 777 +231 - Misses 113 454 +341 ``` | [Impacted Files](https://app.codecov.io/gh/vapor/websocket-kit/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor) | Coverage Δ | | |---|---|---| | [Sources/WebSocketKit/WebSocket.swift](https://app.codecov.io/gh/vapor/websocket-kit/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9XZWJTb2NrZXRLaXQvV2ViU29ja2V0LnN3aWZ0) | `70.52% <37.62%> (-18.52%)` | :arrow_down: | | [Sources/WebSocketKit/PMCE.swift](https://app.codecov.io/gh/vapor/websocket-kit/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9XZWJTb2NrZXRLaXQvUE1DRS5zd2lmdA==) | `39.63% <39.63%> (ø)` | | | [Sources/WebSocketKit/WebSocketClient.swift](https://app.codecov.io/gh/vapor/websocket-kit/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9XZWJTb2NrZXRLaXQvV2ViU29ja2V0Q2xpZW50LnN3aWZ0) | `88.83% <51.72%> (-6.52%)` | :arrow_down: | | [Sources/WebSocketKit/WebSocketHandler.swift](https://app.codecov.io/gh/vapor/websocket-kit/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9XZWJTb2NrZXRLaXQvV2ViU29ja2V0SGFuZGxlci5zd2lmdA==) | `58.44% <52.00%> (-4.06%)` | :arrow_down: | | [...urces/WebSocketKit/HTTPUpgradeRequestHandler.swift](https://app.codecov.io/gh/vapor/websocket-kit/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9XZWJTb2NrZXRLaXQvSFRUUFVwZ3JhZGVSZXF1ZXN0SGFuZGxlci5zd2lmdA==) | `73.77% <100.00%> (+1.84%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/vapor/websocket-kit/pull/142/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor)
jhoughjr commented 1 year ago

The coverage seems odd to drop so much. I only removed a skipped test and added some that werent there. I don't understand that result.

adam-fowler commented 1 year ago

@0xTim You are not serious, are you?

@0xTim is right here to question relying on a package that hasn't hit v1.0. CompressNIO has had a number of breaking changes in the last few releases. Although many of those have been to add support needed by both this PR and the equivalent in hummingbird-websocket.

To make things easier I will move compress-nio to v1.0 in the next week or so.

jhoughjr commented 1 year ago

@adam-fowler Thanks a ton. Your quick responsiveness and guidance has been instrumental in getting this done on time for me.

adam-fowler commented 1 year ago

@jhoughjr I just released compress-nio v1.0 so you can update to that and it won't be an issue anymore

koraykoska commented 1 year ago

Thank you @adam-fowler

jhoughjr commented 1 year ago

Thought I pushed yesterday, seemed today it didnt go so pushed it now.

0xTim commented 1 year ago

Hey @jhoughjr any particular reason this was closed? With the recent changes and compression 1.0 this was getting into a good shape