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

vi · 2019-04-30T16:01:20Z

rust-websocket still depends on legacy hyper 0.10.

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

# Conflicts: # Cargo.toml # websocket-codec/src/codec/ws.rs

kpp · 2019-04-30T16:07:23Z

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 · 2019-04-30T16:13:23Z

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 · 2019-04-30T17:01:06Z

I will help if you need to.

kpp · 2019-05-01T20:02:10Z

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

kpp · 2019-05-01T20:21:01Z

src/result.rs

+	}
+}
+
+pub(crate) fn towse<E>(e: E) -> WebSocketError


Sorry but the name sux. How about using .map_err(WebSocketOtherError::from)?

I'm not sure about how should I do error handing after splitting the crate in general. This function is basically to avoid typing .map_err(WebSocketOtherError::from) and making lines really longer (or introducing additional lines).

Making websocket-lowlevel's error type primary error type also for high-level websocket is dubious, but I'm not sure how to do better: there are traits that are re-exported from websocket-lowlevel by websocket and those rely on lowlevel's error type.

kpp · 2019-05-01T20:22:21Z

websocket-lowlevel/src/codec/ws.rs

-use bytes::BytesMut;
-use tokio_codec::Decoder;
-use tokio_codec::Encoder;
+use self::bytes::BufMut;


Why self::?

Probably some edition 2015 shenanigans.

kpp · 2019-05-01T20:28:05Z

websocket-lowlevel/src/header.rs

+	pub fn new() -> WebSocketKey {
+		let key: [u8; 16] = unsafe {
+			// Much faster than calling random() several times
+			mem::transmute(rand::random::<(u64, u64)>())


How about https://rust-random.github.io/rand/rand/trait.RngCore.html#tymethod.fill_bytes?

This code was only transferred from the parent crate, not invented by me.

Maybe at the time the code was written, rand had fewer functions.

Do you expect "drive by" refactorings to be in scope for splitting pull request?

No, I can do that in another PR, and you can rebase on master to get these changes

kpp · 2019-05-01T20:55:34Z

websocket-lowlevel/src/header.rs

+				let mut array = [0u8; 20];
+				let mut iter = vec.into_iter();
+				for i in &mut array {
+					*i = iter.next().unwrap();


How about:

match base64::decode(accept) { Ok(vec) => { use std::convert::TryInto; vec[..].try_into() .map(WebSocketAccept) .map_err(|_e| WebSocketError::ProtocolError( "Sec-WebSocket-Accept must be 20 bytes", )) } Err(_) => Err(WebSocketError::ProtocolError( "Invalid Sec-WebSocket-Accept ", )), }

vi · 2019-05-01T20:58:16Z

The plan is mostly this:

Split away Hyper_0.10-independent part away to separate crate websocket-lowlevel;
Try implementing new, maybe API-incompatible async-only crate based on websocket-lowlevel, either using hyper 0.12 or using something else.
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.
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 · 2019-05-01T20:59:00Z

websocket-lowlevel/src/header.rs

+	/// Return the Base64 encoding of this WebSocketAccept
+	pub fn serialize(&self) -> String {
+		let WebSocketAccept(accept) = *self;
+		base64::encode(&accept)


pub fn serialize(&self) -> String { base64::encode(&self.0) }

This code was only transferred from the parent crate, not invented by me.

kpp · 2019-05-01T21:05:03Z

websocket-lowlevel/src/codec/ws.rs

@@ -365,6 +368,8 @@ mod tests {

 	#[test]
 	fn message_codec_client_send_receive() {
+		extern crate tokio;


Import this crate only in lib.rs with cfg test

Doesn't #[test] imply #[cfg(test)]?

What's wrong with non-root crate import?

Take a look at https://github.com/websockets-rs/rust-websocket/pull/219/files#diff-b4aea3e418ccdb71239b96952d9cddb6R50

kpp · 2019-05-01T21:08:07Z

src/header/accept.rs


 /// Represents a Sec-WebSocket-Accept header
 #[derive(PartialEq, Clone, Copy)]
-pub struct WebSocketAccept([u8; 20]);
+pub struct WebSocketAccept(WebSocketAcceptLL);


Why? pub use websocket_lowlevel::header::WebSocketAccept would be enough

It does not implement hyper_0.10's Header and HeaderFormat traits.

kpp · 2019-05-02T00:43:47Z

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 · 2019-05-02T00:55:01Z

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 · 2019-05-02T00:57:27Z

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.

Conflicts: src/header/accept.rs src/header/key.rs

ahmedcharles · 2019-08-14T04:25:16Z

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 · 2019-08-14T04:59:04Z

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 · 2019-08-14T05:07:23Z

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

vi · 2019-08-14T05:10:19Z

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

Review it and state opinions:

Is quality of the pull request good enough so better to merge than to wait more?
How hacky / unmanageable error handling becomes? Error handling is the trickiest part of the split. Now it's somewhat "reverse": lowlevel's error is main error type and highlevel's error is dyn-boxed away.
Are there any breaks of dependent crates introduced by this split?

ahmedcharles · 2019-08-14T06:48:14Z

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 · 2019-08-14T10:55:17Z

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 · 2019-08-14T16:46:21Z

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

kpp · 2019-08-14T16:50:05Z

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 · 2019-08-15T04:09:35Z

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 · 2019-08-15T06:35:59Z

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

@vi Are you sure?

vi · 2019-08-31T21:09:27Z

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 · 2019-09-14T20:07:38Z

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 · 2019-10-21T13:42:50Z

@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 · 2019-10-21T20:41:21Z

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 · 2019-10-21T22:32:38Z

@hyyking Which names do you suggest for:

rust-websocket part that does not depend on any HTTP or async library. Just encoding/decoding messages, base64, key, some type definitions, etc.
rust-websocket part that can wrap an async connection to provide a WebSocket message-based async connection, but without negotiation / http upgrade part. That would depend on futures.
rust-websocket part that depends on legacy outdated hyper 0.10 library? Shall it provide fully-fledged WebSocket experience or just HTTP upgrade without the channel itself?

hyyking · 2019-10-22T11:25:49Z

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

rust-websocket-encoding (ws-encoding workspace) for message encoding
rust-websocket-upgrade (ws-upgrade workspace) for the http upgrade protocol (deps: hyper and ws-encoding)
rust-websocket should provide work-ready sockets with sync/async/tls features like it does right now

vi · 2019-10-22T17:05:15Z

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

hyyking · 2019-10-22T18:18:12Z

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.

Conflicts: src/client/builder.rs src/result.rs

vi added 8 commits April 8, 2019 22:34

Add comment to is_secure_url

0f144f9

Begin splitting into two crates

519e885

Merge remote-tracking branch 'origin/master' into split_in_two

5e20b76

# Conflicts: # Cargo.toml # websocket-codec/src/codec/ws.rs

Rename websocket-codec to websocket-lowlevel

6d47562

Fix/ignore some tests

1d2a1ee

Add very minimal doc for websocket-lowlevel.

dfd0353

Extract some hyper-independent header processing to the lowlevel crate

a23a700

Remove extra deps from the highlevel crate.

041a090

vi changed the title ~~Extract hyper-intependent part of rust-websocket into a separate crate.~~ Extract hyper-independent part of rust-websocket into a separate crate. Apr 30, 2019

vi mentioned this pull request Apr 30, 2019

Remove a lot of unnecessary tokio deps (udp, fs, uds and so on) #219

Merged

vi added 2 commits April 30, 2019 19:19

Merge remote-tracking branch 'origin/master' into split_in_two

54475ab

cargo fmt

a941262

kpp reviewed May 1, 2019

View reviewed changes

vi added 2 commits June 10, 2019 01:02

Merge remote-tracking branch 'origin/master' into split_in_two

fad91d5

Conflicts: src/header/accept.rs src/header/key.rs

Fix Rust 1.28 compat

36be61b

vi mentioned this pull request Jul 13, 2019

Added StatusCodeError when receiving unexpected status code #234

Merged

vi added 5 commits November 7, 2019 21:02

Additional comment

d6f715e

Rename websocket-lowlevel to websocket-base

10dbb8c

Elaborate comment in websocket-base

b3e3970

Merge remote-tracking branch 'origin/master' into split_in_two

0b51f58

Conflicts: src/client/builder.rs src/result.rs

Bump version to 0.24.0

e1b811c

vi force-pushed the split_in_two branch from 47d9701 to e1b811c Compare November 7, 2019 18:36

vi merged commit e1b811c into master Nov 7, 2019

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

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

Uh oh!

Conversation

vi commented Apr 30, 2019

Uh oh!

kpp commented Apr 30, 2019

Uh oh!

vi commented Apr 30, 2019

Uh oh!

kpp commented Apr 30, 2019

Uh oh!

kpp commented May 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vi commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpp commented May 2, 2019

Uh oh!

vi commented May 2, 2019

Uh oh!

kpp commented May 2, 2019

Uh oh!

ahmedcharles commented Aug 14, 2019

Uh oh!

vi commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahmedcharles commented Aug 14, 2019

Uh oh!

vi commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahmedcharles commented Aug 14, 2019

Uh oh!

vi commented Aug 14, 2019

Uh oh!

ahmedcharles commented Aug 14, 2019

Uh oh!

kpp commented Aug 14, 2019

Uh oh!

ahmedcharles commented Aug 15, 2019

Uh oh!

kpp commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vi commented May 1, 2019 •

edited

Loading

vi commented Aug 14, 2019 •

edited

Loading

vi commented Aug 14, 2019 •

edited

Loading

kpp commented Aug 15, 2019 •

edited

Loading

vi commented Oct 21, 2019 •

edited

Loading

vi commented Oct 22, 2019 •

edited

Loading

hyyking commented Oct 22, 2019 •

edited

Loading