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

jhoughjr · 2023-07-05T18:43:54Z

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.

…eems to define.

…(from)

0xTim

Some quick initial notes on this - the PR needs tidying up to start with, there's extra whitespace added, commented out code, removed tests etc

The main issue is adding a dependency on something that is not yet 1.0.0 - this is a blocker as we can't guarantee API breaking changes. This will need to be tagged a 1.0.0 before we can consider the PR (or inline the code we need)

koraykoska · 2023-07-06T10:05:59Z

@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 · 2023-07-06T11:42:39Z

Also want to mention this exists

Joannis

Code quality should really be improved, especially logging. And tests need to be added. I think this is a great start for a great addition. I'll have to read up on PMCE, but I think that the implementation is definitely a great addition and pretty well implemented.

Joannis · 2023-07-06T11:43:06Z

Sources/WebSocketKit/WebSocket.swift

    }

    @preconcurrency public func onText(_ callback: @Sendable @escaping (WebSocket, String) -> ()) {
+        print("\(self.onTextCallback._eventLoop)")


Print statements must be removed

PMCE RFC is somewhat obtuse. I'd be very surprised if it were perfectly implemented currently but it has worked against 4 different servers in my lab tests. These are all good suggestions and ive addressed most of them I think.

Joannis · 2023-07-06T11:43:21Z

Sources/WebSocketKit/WebSocket.swift

-        self.channel.writeAndFlush(frame, promise: promise)
-    }
+        //using compression
+        if pmce != nil && pmce!.enabled {


Please if let unwrap this, instead of force unwrapping

Joannis · 2023-07-06T11:44:13Z

Sources/WebSocketKit/WebSocket.swift

+                    }
+                }
+                catch {
+                    print("websocket-kit: \(error)")


Get rid of this print statement, and add some error handling. Of course, you can keep logging in here through Logger

Im not sure what error handling there would be other than logging the underlying error in the catch.

Joannis · 2023-07-06T11:44:23Z

Sources/WebSocketKit/WebSocket.swift

            }
+            else if frame.rsv1 && pmce == nil {
+                if pmce?.logging ?? false {
+                    print("websocket-kit: PMCE:  received compressed frame without PMCE configured! Closing per RFC-7692. You could have a configuration issue.")


Use swift-log

That would mean adding a dep to websocket kit for it I assume. I had looked into it possibly but thought it kinda out of scope and thought if ppl wanted logger there they would have one by now and maybe there was a reason websocket had no logger.

How is logging handled right now in websocket-kit? Simply no logging or with prints? If it's no logging I would keep it that way and make a separate PR eventually adding proper logging and the dependency.

It was no logging. I asked Tim and he agreed it shold be added. I can't make another fork though so I just did it on this PR. It was kind of a pain point to debug it without logging so I added what I could after asking around for a while. But logging has been added via swift log to websocket and pmce classes with a boxed bool to enable with a defeat of false. that way you can turn it on for real time issues without needed a fork to add it.

Logging is added now on main.

Joannis · 2023-07-06T11:45:13Z

Sources/WebSocketKit/WebSocketClient.swift

+            self.maxFrameSize = maxFrameSize
+            self.minNonFinalFragmentSize = 0
+            self.maxAccumulatedFrameCount = Int.max
+            self.maxAccumulatedFrameSize = Int.max


I don't think the max frame sizes should ever be set to .max implicitly.

that might be a good catch. Not sure where I saw that from, thought I pulled it from elsewhere in the codebase.
Do you know what default should be?

I don't have a specific requirement. But I tend to require the API consumer to think about this. In our case - Vapor itself

I feel like I copied it from elsewhere, but could have pulled it from somehwere... I just looked. I got it from the pre existing TLS init I was basing adding pmce arg on public init( tlsConfiguration: TLSConfiguration? = nil, maxFrameSize: Int = 1 << 14 ) so I was using vapor as the guide as far as I could tell.
I do see ur point of the caller should set that but I see no way for them to pass it on init.
Seeing as it's all sendable now, shouldn't those be loop bound to be mutable? That would prevent them from chasing it after the fact.
Im surprised it doesnt warn me. as it did on WebSocket having mutable state while being sendable. am confused.

Joannis · 2023-07-06T11:50:47Z

Tests/WebSocketKitTests/WebSocketKitTests.swift

    override func setUp() async throws {
        fflush(stdout)
    }
+
+    override func tearDown() {
+        try! self.elg.syncShutdownGracefully()


Not a fan of force unwrapped optionals. And try!. You can make tearDown() throwing.

I think those were there originally I dont recall writing them.

Ive removed the !.s I can find. Not sure what to do about the ones some other maintainer left in there.
I can try if its important but feel it could unravel things maybe.

If another user did that, I don't think that's your responsibility to fix it here :)

I dont mind fixing them, but without understanding the test I cant say my change would still be a valid test. I did that in the cases I did change, like with the URL, but didnt want to go down the rabbit hole like a cowboy. Someone should do it I think. Since I already fixed deprecations without asking it's kinda six of one half dozen of another.

Joannis · 2023-07-06T11:50:58Z

Tests/WebSocketKitTests/WebSocketKitTests.swift

@@ -279,42 +287,6 @@ final class WebSocketKitTests: XCTestCase {
        try server.close(mode: .all).wait()
    }

-    func testLocally() throws {


Why was this removed?

I had thought it was something I had added to test locally. It hangs forever the only test that didnt ever pass .

Just adding what I mentioned in discord that I removed it because it was skipped anyway and mistakenly thought had made it due to its vague name as I do that when drafting a test. Especially since not skipped it would hang forever.
Adding it back just adds a skipped taste is the takeaway I htink.

Joannis · 2023-07-06T11:51:15Z

Tests/WebSocketKitTests/WebSocketKitTests.swift

@@ -479,7 +451,9 @@ final class WebSocketKitTests: XCTestCase {
    }

    func testBadURLInWebsocketConnect() async throws {
-        XCTAssertThrowsError(try WebSocket.connect(to: "%w", on: self.elg, onUpgrade: { _ in }).wait()) {
+        // %w seems to now get to NIO and it attempts to connect to localhost:80 ... empty string makes the test pass
+        XCTAssertThrowsError(try WebSocket.connect(to: "", on: self.elg, onUpgrade: { _ in }).wait()) {


Again, let's make sure we don't change tests just to get things passing.

The test is no longer valid and thus did not pass when it in fact should have passed because it wasnt testing what it said it was testing. it expects %w to return nil not a URL if I recall. As a QA engineer I take tests pretty seriously.

I'd add I tested this with a temporary test of URL init with both params and discerning what the test was testing, from that result and the test expectations. So I changed the setup to satisfy the expectation that existed prior.
I was surprised %w returned a URL and not nil.

Joannis · 2023-07-06T11:52:33Z

Tests/WebSocketKitTests/WebSocketKitTests.swift

-    var elg: EventLoopGroup!
+
+
+    // compression usage tests


Let's remove this old (commented) testing code

I just noticed the tests I added to websocket kit dont seem to be here ....!?

Should be fixed now.

Joannis · 2023-07-06T11:53:54Z

Sources/WebSocketKit/WebSocket.swift

 import NIOConcurrencyHelpers

 public final class WebSocket: Sendable {
-    enum PeerType: Sendable {
+
+    public enum PeerType: Sendable {


This doesn't need to be public

I think it's actually needed to be public but I'll check once more. Seems it wa a couple days ago that I changed it so might not have the same api.

I just checked and it needs to be public because types outside Websocket need it.
internal doesnt suffice because an inter needs it. I really think this should be like surface protein for a websocket. It's a perfectly fine enum and is useful for the classes around websocket to support its use.

…nd error where appropriate.

Joannis · 2023-07-06T15:01:54Z

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

…integration tests as well.

codecov-commenter · 2023-07-06T16:53:17Z

Codecov Report

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

❗ 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

@@             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	Coverage Δ
Sources/WebSocketKit/WebSocket.swift	`70.52% <37.62%> (-18.52%)`	⬇️
Sources/WebSocketKit/PMCE.swift	`39.63% <39.63%> (ø)`
Sources/WebSocketKit/WebSocketClient.swift	`88.83% <51.72%> (-6.52%)`	⬇️
Sources/WebSocketKit/WebSocketHandler.swift	`58.44% <52.00%> (-4.06%)`	⬇️
...urces/WebSocketKit/HTTPUpgradeRequestHandler.swift	`73.77% <100.00%> (+1.84%)`	⬆️

... and 1 file with indirect coverage changes

jhoughjr · 2023-07-06T17:43:44Z

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 · 2023-07-06T22:11:35Z

@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 · 2023-07-07T14:35:11Z

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

adam-fowler · 2023-07-12T09:54:15Z

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

koraykoska · 2023-07-12T10:02:28Z

Thank you @adam-fowler

jhoughjr · 2023-07-13T16:13:02Z

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

0xTim · 2023-07-17T23:38:14Z

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

jhoughjr added 15 commits July 2, 2023 09:50

restored

f479096

wip finishing touches for header based Zlib config beyond what pmce s…

7d87990

…eems to define.

wip extended header api

8b0e960

wip

88184d0

wip zlib header optsions.

e0c16f3

wip some notes

2e4f55e

added the zlib fields to header constructor. Loggers ready in configs…

c959ef4

…(from)

wip configs(from)

0dc0bc0

wip handling client with no pmce on a server with pmce available.

82e87bd

wip idk why I bother.

a76388c

wip PMCE enable disable not by if configured but if client requests it.

5e6d46f

some final cleanup before the PR is prepared.

be4a1ea

wip cleanup for PR

a38e2fd

removed unused test idea

f777434

Merge branch 'vapor:main' into main

ecb2dff

0xTim reviewed Jul 5, 2023

View reviewed changes

Joannis requested changes Jul 6, 2023

View reviewed changes

added swift-log to websocket-kit. replaced prints with logger.debug a…

07680dc

…nd error where appropriate.

jhoughjr added 2 commits July 6, 2023 11:12

wip addressing review.

7295fa0

added a few config tests to cover parsingm but its really covered by …

64ae251

…integration tests as well.

jhoughjr added 3 commits July 6, 2023 15:15

removed some comments

44a29cb

cleanup prints

46f258e

removed old comment

66221d6

more cleanup

a6ef521

update to CompressNIO 1.0.0

756216d

jhoughjr closed this by deleting the head repository Jul 15, 2023

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

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

Conversation

jhoughjr commented Jul 5, 2023 • edited by MahdiBM Loading

0xTim left a comment

Choose a reason for hiding this comment

koraykoska commented Jul 6, 2023

Joannis commented Jul 6, 2023

Joannis left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joannis commented Jul 6, 2023

codecov-commenter commented Jul 6, 2023 • edited Loading

Codecov Report

jhoughjr commented Jul 6, 2023

adam-fowler commented Jul 6, 2023

jhoughjr commented Jul 7, 2023

adam-fowler commented Jul 12, 2023

koraykoska commented Jul 12, 2023

jhoughjr commented Jul 13, 2023

0xTim commented Jul 17, 2023

jhoughjr commented Jul 5, 2023 •

edited by MahdiBM

Loading

codecov-commenter commented Jul 6, 2023 •

edited

Loading