rust-socketio
rust-socketio copied to clipboard
Add an async socket.io API
This PR builds upon the async engine.io version and adds an async socket.io interface.
The interface is similar to the sync one and provides most methods in an async fashion. Note that the async version can currently only be used with tokio
.
An Issue I stumbled upon while writing this, is the verbose way of defining callbacks (they need to return boxed futures).
I explained the issue here. From my point of view there is currently no better way than using async { .. }.boxed()
.
Codecov Report
Merging #180 (35ac8b3) into main (37b5592) will decrease coverage by
1.34%
. The diff coverage is79.75%
.
@@ Coverage Diff @@
## main #180 +/- ##
==========================================
- Coverage 86.94% 85.59% -1.35%
==========================================
Files 30 35 +5
Lines 2351 2826 +475
==========================================
+ Hits 2044 2419 +375
- Misses 307 407 +100
Impacted Files | Coverage Δ | |
---|---|---|
engineio/src/asynchronous/client/builder.rs | 91.12% <ø> (ø) |
|
socketio/src/client/builder.rs | 89.39% <0.00%> (-1.38%) |
:arrow_down: |
socketio/src/client/client.rs | 85.52% <0.00%> (-0.29%) |
:arrow_down: |
socketio/src/lib.rs | 100.00% <ø> (ø) |
|
socketio/src/socket.rs | 76.08% <ø> (-0.76%) |
:arrow_down: |
socketio/src/asynchronous/client/callback.rs | 37.50% <37.50%> (ø) |
|
socketio/src/asynchronous/socket.rs | 76.84% <76.84%> (ø) |
|
socketio/src/asynchronous/client/client.rs | 78.22% <78.22%> (ø) |
|
socketio/src/asynchronous/client/builder.rs | 87.50% <87.50%> (ø) |
|
engineio/src/asynchronous/client/async_client.rs | 94.91% <90.00%> (-1.95%) |
:arrow_down: |
... and 14 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Current open points:
- fix tests so that it is ensured that callbacks are called correctly
- ~~investigate UB that might arise when cloning the client~~
- ~~put async callbacks behind a feature flag~~
@nshaaban-cPacket What alternatives to async callbacks would you consider? A possible alternative would be to somehow use the implementation of Stream
to react to a packet and provide means to answer to the server directly. I don't like the way the callbacks work for a two main reasons:
- The fact that the callback takes a
Client
as an argument is the root of the whole "client-needs-to-be-clonable" problem -
Fn
s are hard to handle (especially in async context, where creation ofFn
s is difficult and counterintuitive) also they need to be boxed
I added the async-callbacks
feature flag. As there is currently no other alternative to async callbacks, I added it as a default feature when async
is enabled. Anyhow the ClientBuilder
's on<F: Fn(...) -> ...>(..)
method is excluded when the flag is not enabled, giving an option to add different versions of the method (or other interfaces like trait implementations) as needed.
@nshaaban-cPacket What alternatives to async callbacks would you consider? A possible alternative would be to somehow use the implementation of
Stream
to react to a packet and provide means to answer to the server directly.
Either:
- streams (good in theory, bit of a pain to implement)
- trait based callback. not exactly sure how this works internally, but the idea is provide a trait that implements handle_message (or similar), then pass that object into the client. then users can create their own structs with their own state that handle socket.io messages.
Sorry it took so long to review, didn't see that you re-requested review.
fix tests so that it is ensured that callbacks are called correctly
May want to remove those tests that aren't actually testing so the coverage number is accurate.
Hey, FWIW, I tried out the async version and everything seemed to work except for event callbacks (not sure how comprehensively those are tested). The sync version of the library works fine with the same calling code, so I'm pretty sure it was not a server config problem.
@JackFlukinger I found the callback not await
, so not really executed. @1c3t3a I made a PR #226
https://github.com/SSebo/rust-socketio/blob/78f07ded196e3b6d5b2209ba0fafc86a0f3d0b15/socketio/src/asynchronous/client/client.rs#L232
hi, @1c3t3a :
The fact that the callback takes a
Client
as an argument is the root of the whole "client-needs-to-be-clonable" problem
https://github.com/SSebo/rust-socketio/blob/0b1890e5f7036105ef6661c4be7656af43f0ef16/socketio/src/client/client.rs#L37-L54
For reconnecting, the Client is cloneable, and the real client is inner: Arc<RwLock<Client>>
. because we need to create a new socket to replace failed one, so the whole "client-needs-to-be-clonable" problem is acceptable I think.
What alternatives to async callbacks would you consider?
- Callback in rust is also hard to access outer variable which must be wrapped with
Arc<Lock>
then move into the callback closure. - Register callback is normal in Javascript, but not very popular in Rust. The normal rust way usually register a sender, and spawn a new thread to recv new event, move necessary variable in, then process it.
- If callback panic, it will kill the socket.io thread (or we spawn thread to handle callback), if sender is dead, socket.io thread can just skip it.
What alternatives to async callbacks would you consider?
Another option is rather than sending the client in the callback, send an intermediary object that can uses a message channel (or similar means) to send messages out over the client. Inside the callback you don't really need to have the full powers of the client, just send messages/close connection/etc.