Add permessage-deflate support, again

[SvizelPritula]

In #328, @kazk implemented support for the permessage-deflate extention. Implementing this extention required the ability to parse the Sec-Websocket-Extentions header, so @kazk submitted a pull request to the headers crate. However, the maintainers of the headers crate don't wan't to support this header yet, as they don't wan't to commit to any particular API. As such, they suggested that it should be implemented in this or some other crate, tweaked as necessary and potentially moved into the headers crate in the future.

This PR reverts the commit that reverted @kazk's commit that added permessage-deflate support. It also copies his implementation of SecWebsocketExtentions intended for the headers crate.

The implementation of SecWebsocketExtentions relied on some internal utilities of the headers crate. I've removed some of those dependencies and re-implemented others. I have also gated the implementation behind the existing handshake feature, as well as any code that relies on it, since it relies on the headers crate, which in turn relies on many other HTTP crates.

Blocking issues

• @nakedible-p found an error in the implementation. As I know very little about the Websocket spec, I'm unsure if I can fix it properly.
• The headers crate is licensed under the MIT license, while this crate uses a dual MIT+Apache license. We would need to add a proper license notice, or @kazk would need to consent to the re-licensing of his PR.

Unresolved questions

• It is impossible to use the deflate feature without having tungstenite handle the Websocket handshake. There is a WebSocket::from_raw_socket_with_extensions method to allow for exactly that, but it takes an Extensions struct, which has no public constructor.
• The only way to get such a struct is from the WebSocketConfig::accept_offers method, which implements the server part of extension negotiation and requires the handshake feature. This method is intended to allow for tungstenite to be integrated into web frameworks. No similar public method exists for clients, although that might not be a big issue, since a client knows whether a request will be a Websocket request up front.
• To me, the WebSocketConfig::accept_offers feels out of place on the WebSocketConfig. We could make it a standalone function, perhaps in the extensions module.
• I have made the SecWebsocketExtentions implementation public. This is necessary to make the WebSocketConfig::accept_offers method usable. This does mean that if SecWebsocketExtentions were to be added to headers in the future, switching to that implementation would (I think) be a breaking change. An alternative would be to have WebSocketConfig::accept_offers take and return a HeaderValue, which it would parse itself. This would allow us to make SecWebsocketExtentions a private implementation detail.
• This is a breaking change, since a new field was added to WebSocketConfig and a new variant was added to the Error enum. Since this field and variant depends on the deflate feature, I've annotated both with #[non_exhaustive]. I've done the same to ProtocolError, which currently has variants gated behind handshake. This sadly means that you cannot create WebSocketConfig using the initializer syntax, instead you have to create an instance with WebSocketConfig::default and mutate it afterward. Sadly, I see no way to avoid this. It would be possible to disable #[non_exhaustive] if all relevant features are enabled.

[nakedible-p]

I will be happy to go through the details of the permessage-deflate spec – or rather even fix the bug myself, while some missing window bits support. However, the bug is extremely niche as these extension fields are very rarely used, so I'd prefer to try to get this merged and then do an improvement pull on top of that.

[SvizelPritula]

@kazk Would you be willing to relicense your PR under the MIT + Apache dual license as used by this crate?

If not, I believe we can use the code under MIT + Apache anyways, as long as we add a NOTICE file to the repo that looks a little bit like this:

This crate contains code copied from the headers crate, licensed as follows:

*Insert copy of headers license here.*

Additionaly, we would probably have to add "Copyright (c) 2014-2023 Sean McArthur" to LICENSE-MIT, which is odd, given that no part of the code in this PR was actually written by Sean McArthur.

[zitsen]

What's the status of this pr?

[SvizelPritula]

What's the status of this pr?

I forgot about it, sorry.

I'd like to modify it to meet the necessary licensing obligations, which shouldn't be too hard.

[SvizelPritula]

I'm not a lawyer, but I think that the notices I've to the license files should be compliant with all relevant licenses. As such, this PR is now ready for review! 🥳 Sorry for the delay.

[kazk]

@SvizelPritula

I apologize for the delay.

@kazk Would you be willing to relicense your hyperium/headers#88 under the MIT + Apache dual license as used by this crate?

Yes, you can do whatever is necessary for what I did :)
Thanks for your work!

[zitsen]

Glad to see it still going on, thanks your great work @SvizelPritula @kazk .

So are there any other problems pending this?

Sorry for disturbing, but seems that I should ping you @nakedible-p @daniel-abramov .

[daniel-abramov]

I remember I checked #328 back then, and it seems like there are many common parts (back then it was reverted due to dependencies. So yeah, I'm not against merging it.

I'd also appreciate it if someone who tracked the issue could approve it, though (to ensure that I did not miss anything since I did not go as thoroughly through all the changes as I typically try to do).

[goriunov]

We have been running a copy of this branch for couple of weeks now in production, at least in the standard cases it seems to work good, both directions server client and client server, different languages clients connect to the server.

[daniel-abramov]

We have been running a copy of this branch for couple of weeks now in production, at least in the standard cases it seems to work good, both directions server client and client server, different languages clients connect to the server.

This sounds pretty good! It's always good to know that someone tested it in production!

@SvizelPritula, would you be interested in rebasing on top of a master branch? (there have been quite significant changes recently that affect performance and change the API surface a bit)

[goriunov]

@daniel-abramov @SvizelPritula just checking in. Is there anything else that needs to be done before this branch can be merged?

[SvizelPritula]

just checking in. Is there anything else that needs to be done before this branch can be merged?

I've merged this branch with master, which broke the Autobahn test suite. I'll try to investigate why.

[goriunov]

Just checking in if there are any updates on this PR?

[SvizelPritula]

The tests are now fixed, everything should be working.

Sorry for taking so long, I was busy with exams.

[daniel-abramov]

Thanks for the update!

I had to quickly go through the changes again (albeit skipping all deflate-specific logic) as I keep forgetting the things between reviews (the PR is pretty large).

Generally it looks good, I'm only a bit concerned with a slightly more complicated logic in protocol/** (we have more branching and more conditions), but making it better would possibly require a bigger overhaul.

I also decided to run our benchmarks locally and it seems like this version is somewhat slower than the version in master when it comes to reading. I consistently get a regression of around 15-17%.

read+unmask 100k small messages (server)
                        time:   [6.6915 ms 6.7087 ms 6.7273 ms]
                        change: [+14.968% +15.822% +16.508%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 17 outliers among 100 measurements (17.00%)
  6 (6.00%) low mild
  5 (5.00%) high mild
  6 (6.00%) high severe

read 100k small messages (client)
                        time:   [6.4633 ms 6.4821 ms 6.5024 ms]
                        change: [+17.048% +17.722% +18.370%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

Unfortunately, I did not have time to dig in and investigate the root causes, but I assume that it might be related to the changes around IncompleteMessage / extend_incomplete(), and/or handling of OpData::Continue.

[daniel-abramov]

Suggested change

	autobahn/client/
	autobahn/server/

Probably leftovers from a local testing? :)

[SvizelPritula]

Those folders contain the test results after running scripts/autobahn-client.sh or scripts/autobahn-server.sh. Those shouldn't be commited, so I added them to .gitignore.

[daniel-abramov]

Note that this involves copying the data which might get expensive. Since the original payload is not required after the decompression, it's probably better to use into() to directly convert it into Vec<u8>.