rust-socketio icon indicating copy to clipboard operation
rust-socketio copied to clipboard

Add an async socket.io API

Open 1c3t3a opened this issue 2 years ago • 10 comments

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().

1c3t3a avatar Apr 12 '22 13:04 1c3t3a

Codecov Report

Merging #180 (35ac8b3) into main (37b5592) will decrease coverage by 1.34%. The diff coverage is 79.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.

codecov[bot] avatar Apr 12 '22 13:04 codecov[bot]

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~~

1c3t3a avatar Apr 19 '22 18:04 1c3t3a

@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
  • Fns are hard to handle (especially in async context, where creation of Fns is difficult and counterintuitive) also they need to be boxed

1c3t3a avatar Apr 28 '22 20:04 1c3t3a

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.

1c3t3a avatar Apr 28 '22 21:04 1c3t3a

@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.

ctrlaltf24 avatar Apr 28 '22 21:04 ctrlaltf24

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.

ctrlaltf24 avatar May 17 '22 23:05 ctrlaltf24

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 avatar Jun 09 '22 03:06 JackFlukinger

@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

SSebo avatar Aug 16 '22 10:08 SSebo

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.

SSebo avatar Aug 16 '22 10:08 SSebo

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.

ctrlaltf24 avatar Aug 16 '22 20:08 ctrlaltf24