Switch websocket library

[1c3t3a]

This library currently uses the websocket crate. However the library looks kind of abandoned and might not be the best fit for future plans like async / await support. Currently the library provides async support based on tokio 0.1. This could lead to problems and there are no real plans to get the support running with websocket (see #242). It's also not to be expected that the library will provide support in case we have a problem, so switching the library might be a good idea. A battle proven implementation is provided by tungstenite-rs (async here, blocking here). What do you think @nshaaban-cPacket?

[ctrlaltf24]

Sounds good, might be worth doing before the engineio crate goes public? I'm not interested in implementing the change in the near future (only if it causes headache with 0.5.0).

[1c3t3a]

Alright, I could implement it as soon as refactoring is merged. Also a good point that this should be done before engine.io goes public, then I am not tied to a certain API.

[ctrlaltf24]

Alright, I could implement it as soon as refactoring is merged. Also a good point that this should be done before engine.io goes public, then I am not tied to a certain API.

Doesn't necessary have to be before, but might make sense. Ideally what library we are using internally for websockes/polling would not be exposed anyways.

[ctrlaltf24]

websocket is exposed here

rust-socketio/engineio/src/error.rs

Line 44 in 9b1183b

IncompleteResponseFromWebsocket(#[from] WebSocketError),

Plus we'll need the https://docs.rs/tungstenite/0.15.0/tungstenite/fn.client_tls_with_config.html to continue supporting passing in a TlsConnector

[ctrlaltf24]

May want to escalate this @1c3t3a websocket (latest) depends on hyper 0.10.6 which is vulnerable to RUSTSEC-2021-0079 and RUSTSEC-2021-0078 (found using cargo-audit)

[1c3t3a]

May want to escalate this @1c3t3a websocket (latest) depends on hyper 0.10.6 which is vulnerable to RUSTSEC-2021-0079 and RUSTSEC-2021-0078 (found using cargo-audit)

This is actually really bad... I've looked into it and the only solution for websocket would be to update to a hyper version>= 0.14.10`. But this introduces quite massive changes as hyper changed it's entire API around headers and header parsing and multiple other stuff. I started to fix this on my fork but ran into bigger problems. For know I am going to open an issue at websocket (see #262), but I wouldn't necessarily be sure that a fix will be submitted soon. For rust_socketio it would maybe make sense to switch to tungstenite, despite the loss in performance. But let's see how the reaction is at websocket.

[vi]

websocket crate is basically unmaintained now. Switching to tungstenite is recommended.

---

Does it make sense to release a new version of websocket crate that just points to (reexports) tungstenite and tokio-tungstenite, to prevent new accidental dependencies on the unmaintained code? Would benefits outweigh the harm? Currently websocket crate contains a warning in README:

Note: Maintainership of this project is slugglish. You may want to use tungstenite or tokio-tungstenite instead.

Have you missed it when you were shopping around for a Websocket library?

[1c3t3a]

Hi @vi, first of all thanks for the quick help! We originally wanted to switch to tungstenite, but the problem is that the sync version of it is by far slower than websocket.

Observed significant performance degradation (25s minimum, peak of 75s) in the following case (client::socket::test::socket_io_integration):

• Callback mode (where there is a separate thread constantly calling poll)
• Attempt to send packets on the clone of the socket returned
• Observe that websocket connection is busy reading
• When a ping packet is received it is possible that the lock switches to the write
• Socket finally sends a message

This is due to sync tungstenite not supporting the .split operation on their sender / receiver stream, something that websocket is capable of. The long time goal is definitely to switch to tokio-tungstenite (which is way faster), but we didn't make the switch yet, as this requires quite a lot of restructuring internally.

When I started the project about a year ago I didn't see the warning on web sockets Readme (I don't know if it wasn't there of if I just missed it), but the API seemed fine and the project was a bit more vivid then now.

[vi]

This is due to sync tungstenite not supporting the .split operation on their sender / receiver stream

Note that rust-websockets support for splitting sync sockets is only reliable only for non-encrypted ws:// websockets. Maybe that's why sync tungstenite does not expose this function.

Splitting should work properly for async, including for wss://, both in rust-websocket and tokio-tungstenite.

[vi]

When I started the project about a year ago I didn't see the warning on web sockets Readme (I don't know if it wasn't there of if I just missed it)

It is more than a year ago: websockets-rs/rust-websocket@b4f50dd

[1c3t3a]

It is more than a year ago: websockets-rs/rust-websocket@b4f50dd

Then I missed it :D

[1c3t3a]

This is due to sync tungstenite not supporting the .split operation on their sender / receiver stream

Note that rust-websockets support for splitting sync sockets is only reliable only for non-encrypted ws:// websockets. Maybe that's why sync tungstenite does not expose this function.

Splitting should work properly for async, including for wss://, both in rust-websocket and tokio-tungstenite.

Yeah that's true, but it still feels like this is taking away potential speed. But we might change this, @nshaaban-cPacket suggested to internally use async to speed things up, that might now be the best solution.

[ctrlaltf24]

Yeah that's true, but it still feels like this is taking away potential speed. But we might change this, @nshaaban-cPacket suggested to internally use async to speed things up, that might now be the best solution.

Using async internally has it's drawbacks too, we'd have to use a tokio runner (term might be wrong) internally for method calls, which is far from ideal, not to mention also has a performance hit. We may be able to support roughly the same API in asnyc (as most of it is callback based anyways), would require more thought.

[armoha]

There is also async-tungstenite here, which is not bound to any specific async runtime.