Add SecWebsocketExtensions

[kazk]

I'm working on seanmonstar/warp#770 and noticed this is missing.

[seanmonstar]

Thanks for the PR!

Does this type by itself provide any use? I imagine you need to do more with it than simply materialize an opaque value from the map.

[seanmonstar]

Wow, this is a lot of work, thanks for putting so much thought into this!

[seanmonstar]

We should keep the fields private, so we can change the internal implementation whenever desired.

[kazk]

I explained the reason behind this in #88 (comment)

[seanmonstar]

This just a thought, let me know what you think. What if this type simply held a reference, instead of individual owned value strings and a vector of params? The name and params methods could return "lazy" values, as a few of the other headers in this crate already do.

[kazk]

I actually had struct SecWebsocketExtensions(HeaderValueString); with lazy iter() originally. But I wanted decode to fail on values with invalid syntax, so typed_try_get can be used to detect them. This requires parsing the values, so I thought it was better to store the result.

The main reason this can be useful is, when negotiating extensions, values with invalid syntax must be rejected:

If a value is received by either the client or the server during negotiation that does not conform to the ABNF below, the recipient of such malformed data MUST immediately Fail the WebSocket Connection.

https://datatracker.ietf.org/doc/html/rfc6455#section-9.1

If we validate the syntax in decode, crates using headers can focus on the parameters for each extension, and warp can reject the invalid headers, etc..

---

Also, having WebsocketExtension independent is useful because the server needs to filter them, and respond with potentially mutated versions (e.g., parameter ("client_max_window_bits", Some("10")) may be changed to have value Some("9") in the response). WebsocketExtension can provide setters/builder that makes sure that the value won't result in invalid syntax.

This is why I made the field and the struct public, but I can try to come up with something else if this is an issue.

[seanmonstar]

Similar question here, what if instead of owning a Vec, the return iterator were lazy, and did the parsing as requested on each call to next, like other headers?

[ilammy]

I'd like to take over pushing permessage-deflate through the WebSockets stack.

Which review points have to be addressed in order to get this PR merged?

[kazk]

@ilammy I was waiting on a response from @seanmonstar for #88 (comment).

Once this is merged, I already have a branch using it, which is a rebased version of my original tungstenite-rs PR.

[gallowsmaker]

@kazk @seanmonstar I'm humble ask to return to review this PR as dependent functionality in tungstenite-rs extremely important for my team, but in same time we have strict policy don't use libraries from branches, so we have to wait for finalized published version.

[kazk]

@gallowsmaker I can't do anything for this PR, but I'll open a new PR for tungstenite-rs so we can start reviewing the new version while we wait for this. It will also clarify what needs to be done. Looks like I need to rebase again and there are some conflicts, but I should be able to open it soon.

I've been using the original version in production without any issues, but I'd love to get these merged and move on.

[nicknotfun]

@seanmonstar just checking if you're still able to review this?

[nakedible-p]

It would be super nice to get this merged - any hope for this to progress?

[ilammy]

@seanmonstar @kazk bump. What's the blocker here? What help do you need?

[kaszperro]

This PR is highly needed feature. It is blocker for us for using permessage-deflate. What can I do, to speedup merging of this?

[ddimaria]

@seanmonstar are there any plans for this to be merged, or do you require any changes to be made? Getting this merged unblocks a PR in tungstenite that provides permessage-deflate support for websockets.

[AdrianEddy]

@seanmonstar What would it take to get this merged? I'm willing to sponsor this work

[seanmonstar]

I know there's a lot of interest here. I've describe this a few times in chat, but never here. So, let me try:

• This crate tries to expose built-in header types which expose as little internal details as possible. The reason for that is to try to limit mistakes that occurred when this was hyper::header in older versions, and changes to the exposed internals would mean breaking changes.
• It's not clear whether SecWebsocketExtensions should be optimized for parsing, or for mutating.
  • That changes whether it should internally hold onto a flattened string, and parse on the fly of fn iter() -> impl Iterator, or if it should store everything parsed at construction in a list.
  • Unfortunately, that internal decision seems like it will leak, since the iterator would either provide WebsocketExtension<'a>s referring to the flat string, or &WebsocketExtension if an owned item inside a list.
  • Most asking for this feature are more interested in it just working (understandably), than the maintenance details afterwards.
• This doesn't need to block people. This is the most important thing that is missed. Since the crate is based on a trait Header, any other code is able to declare its own typed header and impl Header for MySecWebSocketExtensions.
  • That code can start in tungstenite, or wherever else that's wanting this, and the design and performance of the type can be tested and tweaked there.
  • Once those questions are answered and tested, we can merge something here for longer term maintenance.