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

Merge changes from enzious/rust-websocket #192

Open marcelbuesing opened 6 years ago

marcelbuesing commented 6 years ago

I think it would be great to have the changes from enzious/rust-websocket back in websockets-rs/rust-websocket and published as a new crate version.

Major benefit is it contains the tokio reform changes.

Before being merged I think this still needs https://github.com/enzious/rust-websocket/pull/5/files to remove the dependency to git = "https://github.com/enzious/tokio-tls".

I have also created an issue within the project but there appears to be no response. https://github.com/enzious/rust-websocket/issues/6

vi commented 6 years ago

I'll do that in about week and a half if primary rust-websocket developers stay inactive.

marcelbuesing commented 6 years ago

That would be great. I recently attempted to rebase the changes for a merge request but due to e.g. rustfmts etc. it's quite tough. Let me know if I should attempt again to rebase and create a merge request, I just saw that you also want to look into the other merge requests so I assume this will mean quite some work.

vi commented 6 years ago

The more work, the less pull requests would be in. "Tokio reform" (and in general dependency version updates) is although a priority one.

attempted to rebase the changes for a merge request but due to e.g. rustfmts etc. it's quite tough.

Have you succeed in the end? If yes, it may be worth submitting a new pull request with this.

enzious commented 6 years ago

I merged those PRs and did some lint changes. I'm on the clock though so I didn't do a lot. Also of note, I didn't just upgrade tokio. I also changed the way the request is parsed quite a bit by adding the http crate. Someone might want to put their eyes on that. Also, use_extensions is not implemented.

marcelbuesing commented 6 years ago

@enzious thanks for that! Just to mention that the http changes are due hyper v12 using the http types right? So this is another great feature in the merge request. I'll try to find some time to look deeper into those changes.

replace types with those from http crate (3cd48b45) Hyper Changelog

vi commented 6 years ago

Is pull request #186 related to this somehow?

marcelbuesing commented 5 years ago

Since the tokio reform changes and tokio-tls changes have been incorporated now in websocket-rs I feel the major remaining part is the hyper 0.12 upgrade that could still be incorporated into websocket-rs. If I remember correctly hyper 0.12 made quite some significant changes e.g. moving out the Status codes into a separate crate... All of them have been documented here: https://github.com/hyperium/hyper/blob/master/CHANGELOG.md

Resolving the merge conflicts between the fork and rust-websocket by now seems to be almost the same works as doing a hyper migration again. I briefly looked at the merge conflicts today. Nevertheless a major part of the Rust ecosystem seems to already have migrated to hyper 0.12

marcelbuesing commented 5 years ago

I would probably close this issue in favour of an issue: "Migrate to Hyper v0.12"

vi commented 5 years ago

I also made a weak attempt at migrating it to Hyper 0.12 (and also familiarized myself about hyper a bit). It's nowhere near complete although.

New hyper seems to be async-centric and rust-websocket is both for sync and async. I'm not sure how to handle it. Is depending on tokio-current-thread to provide sync mode a good idea?

marcelbuesing commented 5 years ago

Yeah you seem to be correct regarding the observation that Hyper 0.12 is async-centric. I assume any kind of tokio dependency in a sync code would seem weird. Just checked could it be that the http crate used by hyper 0.12 would be a better idea for sync code? Anyway I kind of feel in long term it might make sense to split the async part into a separate crate (it could remain part of the workspace) otherwise it seems dealing with those diverging requirements might become difficult in the future. Sticking to the old version of hyper I assume will in longterm lead to problems with other crates.

Just tested the enzious fork with 

cargo build --example "server" --no-default-features --features "sync"

and due a tokio-io dependencies in the codec module this currently will not build.

vi commented 5 years ago

Usage of hyper 0.10 is already a problem for Websocat: for inclusion in Debian repositories it needs to depend on 0.12.

vi commented 5 years ago

any kind of tokio dependency in a sync code would seem weird

Do you think code suddently starting using epoll (and other platform's analogues) while previously avoiding it is a breaking enough change?

Just checked could it be that the http crate used by hyper 0.12 would be a better idea for sync code?

I assume http crates contains no implementations (i.e. a parser from a byte stream/buffer to a HTTP request/response), just types and definitions.

tinaun commented 5 years ago

Personally i don't see the problem with wrapping async internals in a sync api (similar to what reqwest does) provided there are enough api knobs to control the internal executor.