search

websockets-rs / rust-websocket

A WebSocket (RFC6455) library written in Rust
http://websockets-rs.github.io/rust-websocket/
MIT License
1.55k stars 223 forks source link

Extract hyper-independent part of rust-websocket into a separate crate. #222

Closed vi closed 5 years ago

vi commented 5 years ago

rust-websocket still depends on legacy hyper 0.10.

This may be the first step to migrate it to something else.

kpp commented 5 years ago

Are you by the way interested in #222 (and in general in migration away from hyper 0.10)?

@vi why move from hyper 0.10?

vi commented 5 years ago

Because of it is non-last version of hyper, making it auto-deprecated.

For example, Debian package suites are supposed to contain only one package per crate.

If Hyper 0.10 continues to live, it should be published with some new separate name.

kpp commented 5 years ago

I will help if you need to.

kpp commented 5 years ago

What exactly do you want to achieve? What is your plan?

vi commented 5 years ago

The plan is mostly this:

  1. Split away Hyper_0.10-independent part away to separate crate websocket-lowlevel;
  2. Try implementing new, maybe API-incompatible async-only crate based on websocket-lowlevel, either using hyper 0.12 or using something else.
  3. Use it for my project websocat (which is why I got involved in rust-websocket project in the first place), removing obstacle for publishing to Debian.
  4. Maybe mark websocket as officially deprecated; maybe rewire it to depend on some successor of hyper 0.10 if there is any.

I'm not sure about this although.

kpp commented 5 years ago

So looks like you want to move all core parts and create some kind of https://github.com/snapview/tungstenite-rs/tree/master/src ?

vi commented 5 years ago

tungstenite? I haven't yet considered it. It seems to be relatively new project.

Can it do async? How does it compare to rust-websocket?

kpp commented 5 years ago

Can it do async? How does it compare to rust-websocket?

I don't know, but I have just discovered it in https://github.com/seanmonstar/warp/blob/master/src/filters/ws.rs when googled how to set up hyper with websockets.

ahmedcharles commented 5 years ago

Is this change still being considered?

I'd like to update websockets to the new futures api, but doing that would conflict with this change heavily.

vi commented 5 years ago

Yes, I'm going to base websocat 2.0 on it (in order to drop hyper 0.10 dependency).

You can look at split_in_two branch and point a pull request to there.

But expect the pull request to hang around for rather long time. Maybe it would make sense to release it to crates.io with other name (like websocket-async + websocket-async-ll) in meantime. README of this project may include link to the fork.

ahmedcharles commented 5 years ago

Is there any way I can help with getting it merged?

vi commented 5 years ago

Is there any way I can help with getting it merged?

Review it and state opinions:

ahmedcharles commented 5 years ago

I'm curious, why not just have the crate use a newer version of hyper? After reading the comments here, it seems like the motivation for the split is remaining on hyper 0.10, but no one says why.

vi commented 5 years ago

why not just have the crate use a newer version of hyper?

Hyper 0.12 is drastically different from 0.10. Also it does not support non-async at all. It may be easier to rewrite websocket entirely based on hyper 0.12 and websocket-lowlevel rather than trying to migrate it evolutionally.

ahmedcharles commented 5 years ago

Note, reqwest implements both sync and async apis on top of hyper's async api's. That seems reasonable?

kpp commented 5 years ago

That seems reasonable?

You have to have an executor (futures/tokio/romio) to block on a async future. And this library is executor-free.

ahmedcharles commented 5 years ago

hyper depends on tokio, tokio has an executor, so a sync version of websockets can be implemented on top of hyper by blocking on the tokio executor?

kpp commented 5 years ago

Hyper 0.12 is drastically different from 0.10. Also it does not support non-async at all. I

@vi Are you sure?

vi commented 5 years ago

Is there any way I can help with getting it merged?

Review it and state opinions.

So, do you have any opinions? Shall websocket-lowlevel + websocket be published or there are blockers?

najamelan commented 5 years ago

I have said it before on tungstenite and tide, but I think what would make sense for the rust websocket ecosystem is to have a separate websocket_handshake crate that everyone can agree on.

It would deal with http handshake and websocket crates could take it from when the handshake is finished. It could rely just on http crate, take in something that implements std::Read + std::Write and return that once the handshake is complete. It would deal with protocols, extensions, http2 etc.

All web frameworks could depend on it for the handshake before switching to a websocket phase 2 crate, or implementing their own phase 2 API (like warp)

All websocket crates could depend on it for standalone websocket server.

This would reduce all of the duplicated work, as well as decrease risk of bugs and security vulnerabilities and improve interoperability.

vi commented 5 years ago

@hyyking , Do you have an opinion about this pull request? Shall I release it already (as 0.24.0 or 0.25.0) or something needs to be addressed before that?

hyyking commented 5 years ago

Rust wise LGTM, I don't like the name websocket-lowlevel (maybe websocket-basis/base). I think it's good for now.

But like said above we could split the crate even more it would make feature developping much easier (rn it's hard to differentiate what needs what). Maybe have a codec workspace for async and a true base crate with handshake/frame like said above.

vi commented 5 years ago

@hyyking Which names do you suggest for:

hyyking commented 5 years ago

I believe the project should be split in 3 parts (I'm ok with helping btw)

  1. rust-websocket-encoding (ws-encoding workspace) for message encoding
  2. rust-websocket-upgrade (ws-upgrade workspace) for the http upgrade protocol (deps: hyper and ws-encoding)
  3. rust-websocket should provide work-ready sockets with sync/async/tls features like it does right now
vi commented 5 years ago

Shall special effort be dedicated to prevent / minimize API breakage during the split? Is API preservation worth complicating the error handling?

hyyking commented 5 years ago

Shall special effort be dedicated to prevent / minimize API breakage during the split? Is API preservation worth complicating the error handling?

That's up to you, if we achieve the changes above it's a major release-worthy so breaking changes may occur. Current API is fine imho but maybe things will be uncovered during the split. If these changes go through we should be able to handle errors in a clearer way but that should be another release I think.