Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async/await support #242

Open
frol opened this issue Aug 19, 2019 · 34 comments
Open

Async/await support #242

frol opened this issue Aug 19, 2019 · 34 comments

Comments

@frol
Copy link

frol commented Aug 19, 2019

Do you have a roadmap as to the async/await-based API? I think it will require migrating to 2018 edition and other breaking changes, so I wonder what the plan is.

@vi
Copy link
Member

vi commented Aug 19, 2019

I'm not sure.

Obviously, it should eventually support tokio > 0.1 (I assume that counts as supporting async/await), but development of this project is sluggish.

Is it proper for rust-websocket to try bashing in tokio 0.2 while still depending on legacy hyper 0.10?

Unless some other contributor steps in, it's unlikely that rust-websocket would support tokio 0.2 this year.

@jhpratt
Copy link

jhpratt commented Aug 30, 2019

@vi I can see what I can do by myself. Would you want to remove the synchronous library? I'd also update hyper, as it seems the sensible thing to do.

@vi
Copy link
Member

vi commented Aug 31, 2019

This all may result in a new websocket library with only partial resemblance of rust-websocket.

If it works well, it may be a good idea to grant the crate name websocket (version 1.x) to it and remane this library to websocket-legacy.

@jhpratt
Copy link

jhpratt commented Aug 31, 2019

Taking that as a yes. I wouldn't jump immediately to a 1.0, but it'll obviously be a breaking change.

@vi
Copy link
Member

vi commented Aug 31, 2019

Would you want to remove the synchronous library?

Not sure. Maybe synchronous version can just use current thread executor under the hood, but still support user-facing API as if it is a pre-async world.

There may be multiple websocket-* crates (low-level functions, async version, sync version that uses async and websocket with just re-exports or gives some additions).

@jhpratt
Copy link

jhpratt commented Aug 31, 2019

Ok, that could certainly work. I'm going to attempt a conversion to async, initially scrapping the synchronous API. If that works, it shouldn't be too difficult to rebuild the sync functions, just using block_on.

@vi
Copy link
Member

vi commented Aug 31, 2019

You may want to use (currently unpublished) websocket-lowlevel crate for Hyper-free part of rust-websocket.

@sdroege
Copy link

sdroege commented Oct 28, 2019

Is there any update on this, anybody working on this? I could probably spend some time on updating current git master to std futures and tokio 0.2 if there's interest in it, but I won't have time for doing any other refactoring and reorganization of the crate based on the WIP (?) split_in_two branch.

@sdroege
Copy link

sdroege commented Oct 29, 2019

It seems the split_in_two branch is actually more or less release-ready? In that case I'll base my work on that when I get to it.

@vi
Copy link
Member

vi commented Oct 29, 2019

Recent comments suggest to rename things and maybe further split away futures-independent part.

But anyway if I personally continue something about rust-websocket, it would be that branch. Originally I planned to publish websocket-lowlevel with websocat 2.0, but enthusiasm dwindled and both projects stalled for a while.

@sdroege
Copy link

sdroege commented Oct 30, 2019

Ok, no worries :) Then I'll start work based on the branch and send you a PR for that once it's working.

Would you want to publish it on crates.io as is then or would you block on splitting it further? From what I understand the goal would be to split into a) a base crate, b) futures support (but not tokio-specific?), c) something to glue things together with hyper?

Also do you want to continue maintaining this project at all or would you be looking for someone to take it over or a co-maintainer?

@vi
Copy link
Member

vi commented Oct 30, 2019

Even if async+edition2018+split version is already ready, I'm going to publish an intermediate version which is already split, but still supports old Rust, old edition and old futures-0.1.

Only the websocket-base crate may end up to be shared between those old and new worlds...

And if to drop old Rust compat then API may be changed drastically as well to be more suitable for hyper 0.12 / http.

@sdroege
Copy link

sdroege commented Oct 30, 2019

Ok, so I'll start with the port to new futures/tokio based on the split branch and then we'll see where we end up :)

@sdroege
Copy link

sdroege commented Nov 3, 2019

I have an initial version here https://github.com/sdroege/rust-websocket/tree/tokio-0.2

Covers the lowlevel crate and the tests are passing. I'm cleaning up the tests now and update the docs, then to the highlevel crate.

@sdroege
Copy link

sdroege commented Nov 3, 2019

And also updated the docs/tests to async/await now. Looks much nicer now if you ask me.

@sdroege
Copy link

sdroege commented Nov 3, 2019

So when going through the highlevel crate, I was wondering if all of that should be kept. Based on the lowlevel crate and hyper it seems like the functionality of the highlevel crate can be implemented in a few dozen lines of code.

I'm not going to port the highlevel crate to new tokio at this point because of time constraints, and we should probably first of all decide how the whole API should look like at that point before porting.

@sdroege
Copy link

sdroege commented Nov 3, 2019

A simple example without proper error handling for that can be found here: https://gist.github.com/sdroege/b1a6cc33ff9a81ae3b52125aeb9aa9c5

@vi
Copy link
Member

vi commented Nov 3, 2019

@sdroege Are you interested in being a maintainer of rust-websocket?

@sdroege
Copy link

sdroege commented Nov 4, 2019

@sdroege Are you interested in being a maintainer of rust-websocket?

I already have a lot of other crates and libraries in other languages to maintain :) I can definitely help out a bit, but I think to move forward here we first of all need to decide on a general direction for this crate.

Personally I would

  • Remove the sync and TLS parts from the lowlevel crate
  • Make tokio an optional dependency of the lowlevel crate to also work with other runtimes (async-std)
  • Retire the high-level crate and do separate crates for: a) hyper, b) some minimal custom HTTP implementation, c) whatever other HTTP crates might appear at some point that allow upgrading the connection

@vi
Copy link
Member

vi commented Nov 4, 2019

For websocat I'm going to use my https://github.com/vi/http-bytes as a "minimal custom HTTP implementation".

@vi
Copy link
Member

vi commented Nov 4, 2019

Make tokio an optional dependency of the lowlevel crate to also work with other runtimes (async-std)

Can't it be executor-independent library without explicit mentions of Tokio or other framework?

@sdroege
Copy link

sdroege commented Nov 4, 2019

Can't it be executor-independent library without explicit mentions of Tokio or other framework?

The problem is that tokio has its own AsyncRead/AsyncWrite... otherwise yes. See tokio-rs/tokio#1297

By having optional tokio support you could include a Decoder/Encoder but that's also easy enough to build out of other API.

For websocat I'm going to use my https://github.com/vi/http-bytes as a "minimal custom HTTP implementation".

Perfect, so maybe we could simply have a websocket-hyper and websocket-http-bytes crate on top of websocket-lowlevel?

@vi
Copy link
Member

vi commented Nov 4, 2019

Is there a multi-runtime analogue of tokio-codec?

(I'm not very aware of post-tokio 0.1 async world. Is this async-std vs tokio split really necessary? Can't one just use the other?)

@sdroege
Copy link

sdroege commented Nov 4, 2019

Is there a multi-runtime analogue of tokio-codec?

Unfortunately not yet. OTOH it's quite easy to implement on top of AsyncRead/AsyncWrite yourself so maybe it does not matter.

IMHO the main problem right now is that tokio has its own AsyncRead/AsyncWrite

I'm not very aware of post-tokio 0.1 async world. Is this async-std vs tokio split really necessary? Can't one just use the other?

They have different reactors and require their specific reactor to work. Having both is unfortunate but I hope this will solve itself over time...

I think we can make websocket-lowlevel runtime agnostic though once tokio stops having their own traits. Then we only need the per-http-impl subcrates and all is well.

FWIW: I have a full hyper client implementation now with error handling and everything, that could become the base of the websocket-hyper crate. Only need to do the server side at some point.

@sdroege
Copy link

sdroege commented Nov 4, 2019

I've added the hyper thing as a sub-crate in https://github.com/sdroege/rust-websocket/tree/tokio-0.2 in any case.

@frol
Copy link
Author

frol commented Nov 4, 2019

I beg your pardon for my ignorance, but I wonder what are the key differences between this project and tungstenite & tokio-tungstenite, which has already implemented async/await support.

@vi
Copy link
Member

vi commented Nov 5, 2019

@frol, I haven't tried [tokio-]tungstenite yet, but when I was choosing a websocket library for websocat (sync version, there was no tokio at that time), I rejected ws library because of no backpressure. rust-websocket was OK in that regard, so I stuck with it.

Existence of tungstenite may be the reason of to keep API stability of rust-websocket (because of it API is changed then projects might as well as go tungstenite instead), or just to officially deprecate rust-websocket, if tungstenite covers all use cases; or maybe create a rust-websocket-like API compatibility wrapper over tungstenite.

@sdroege, Have you tried tungstenite?

@sdroege
Copy link

sdroege commented Nov 5, 2019

Last time I checked tungestenite (or maybe I'm confused?) it required running a separate thread and couldn't be directly integrated with a futures executor. Maybe that changed though.

tokio-tungestenite is currently using old tokio 0.1 and has no async/await support, that's why I didn't look into it again recently. I didn't see that there was a PR (or maybe it wasn't there back then yet).

@sdroege
Copy link

sdroege commented Nov 5, 2019

Last time I checked tungestenite (or maybe I'm confused?) it required running a separate thread and couldn't be directly integrated with a futures executor. Maybe that changed though.

I checked the tokio-tungstenite PR now and that works very well and does not involve adding a new thread. The API is also very similar to what we have here, so maybe this can be deprecated in favor of tokio-tungstenite indeed.

@vi
Copy link
Member

vi commented Nov 5, 2019

Added a note about tungstenine to README for now.

@vi
Copy link
Member

vi commented Nov 7, 2019

Note: merged split_in_two into master, soon to be published as 0.24.0 to crates.io.

This is an intermediate version with not all pending PR merged, mostly for gradual upgrading.

Further splits, edition2018, dropping very old rustc support, futures 0.3, etc. are for 0.26.0. (I'll probably leave 0.25 unoccupied to leave some room for compatibility / gradual upgrade tricks in case of they are needed).

@cheblin
Copy link

cheblin commented Nov 19, 2019

Guys I have a question regarding mentioned crates architecture.
I look an tungstenite WebSocket documentation, and I see that this object has two methods:

  • pub fn write_message(&mut self, message: Message) -> and
  • pub fn read_message(&mut self) ->

But Rust borrow checker, should prevent to read and write on same mutable object in different threads at the same time. Is it mean that tungstenite is only half-duplex?

rust-websocket split channel in two parts - receiver and sender and this is seems to me better design . tokio discuss this problem to ( Split I/O resources paragraph )

@frol
Copy link
Author

frol commented Nov 19, 2019

@cheblin I am not familiar with the released/stable tungstenite, but tokio-tungstenite with tokio 0.2 support has the split method, and it works.

@sdroege
Copy link

sdroege commented Nov 19, 2019

That works by placing everything into an (async) mutex IIRC, so that only one thread at a time can access the socket. In theory for a plain TCP connection (not TLS) it would be thread-safe to write and read separately on different threads at the same time but not in general.

There's also async-tungstenite now btw (which I'm currently maintaining), which is a fork of tokio-tungstenite that is not bound to any specific runtime but has optional support for async-std.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants