Add an async socket.io API #180

1c3t3a · 2022-04-12T13:46:05Z

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

The new code and types added by this commit build upon the structure of the asynchronous engine.io version and expose a new AsyncClient and AsyncClientBuilder

codecov · 2022-04-12T13:55:44Z

Codecov Report

Merging #180 (e8032e6) into main (13f8e96) will decrease coverage by 0.44%.
The diff coverage is 87.22%.

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
- Coverage   91.31%   90.87%   -0.45%     
==========================================
  Files          31       37       +6     
  Lines        3764     4535     +771     
==========================================
+ Hits         3437     4121     +684     
- Misses        327      414      +87

Impacted Files	Coverage Δ
engineio/src/asynchronous/client/builder.rs	`98.40% <ø> (ø)`
socketio/src/asynchronous/client/ack.rs	`0.00% <0.00%> (ø)`
socketio/src/lib.rs	`100.00% <ø> (ø)`
socketio/src/socket.rs	`87.39% <ø> (ø)`
socketio/src/asynchronous/client/callback.rs	`64.70% <64.70%> (ø)`
socketio/src/asynchronous/client/client.rs	`85.90% <85.90%> (ø)`
socketio/src/asynchronous/socket.rs	`86.30% <86.30%> (ø)`
socketio/src/asynchronous/client/builder.rs	`90.69% <90.69%> (ø)`
engineio/src/asynchronous/async_socket.rs	`80.50% <100.00%> (+2.51%)`	⬆️
engineio/src/asynchronous/client/async_client.rs	`96.48% <100.00%> (+0.73%)`	⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ctrlaltf24

main concerns:

consider async callback in a separate feature
tests don't test to make sure the callbacks function
version bump

.github/workflows/build.yml

engineio/src/asynchronous/transport.rs

socketio/Cargo.toml

ctrlaltf24 · 2022-04-12T14:32:48Z

socketio/Cargo.toml

+
+[features]
+default = ["async"] # TODO: remove this before merging, just here so that test run through
+async = ["rust_engineio/async", "tokio", "futures-util", "async-stream"]


suggestion: Put async callbacks behind a feature. Personally don't find async callbacks very ergonomic. Putting them behind a feature would allow us to explore other alternatives in a non-breaking fashion.

socketio/src/asynchronous/client/builder.rs

socketio/src/asynchronous/generator.rs

socketio/src/asynchronous/client/client.rs

socketio/src/asynchronous/socket.rs

1c3t3a · 2022-04-19T18:02:36Z

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 · 2022-04-28T20:49:40Z

@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 · 2022-04-28T21:21:07Z

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.

ctrlaltf24 · 2022-04-28T21:33:46Z

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

socketio/src/asynchronous/client/client.rs

ctrlaltf24 · 2022-05-17T23:30:25Z

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 · 2022-05-23T16:34:15Z

socketio/src/asynchronous/client/client.rs

+                        .map(|value| match value {
+                            Value::String(ev) => ev,
+                            _ => "message",
+                        })
+                        .unwrap_or("message")
+                        .into(),


Suggested change

.map(|value| match value {

Value::String(ev) => ev,

_ => "message",

})

.unwrap_or("message")

.into(),

.map(|value| match value {

Value::String(ev) => Event::from(ev),

_ => Event::Message,

})

.unwrap_or(Event::Message),

JackFlukinger · 2022-06-09T03:40:50Z

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.

SSebo · 2022-08-16T10:06:15Z

@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 · 2022-08-16T10:43:03Z

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.

ctrlaltf24 · 2022-08-16T20:19:12Z

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.

….io client

…d socket.io

…s feature

1c3t3a · 2023-01-15T12:31:11Z

Thanks for the valuable discussion in this PR! I am going to merge this PR now, it comes with fairly good documentation of the new async part and I hope to get some feedback on the async API eventually (especially regarding the valuable points here raised by @SSebo and @nshaaban-cPacket). I am releasing this as an alpha version, since we want to be flexible with API changes, the async part is behind a feature flag either way :) Thanks to everyone that was involved in this, whether it was through testing or reviewing this PR! :)

1c3t3a added 3 commits April 12, 2022 15:27

feat: Add an async socketio version

63f96f6

The new code and types added by this commit build upon the structure of the asynchronous engine.io version and expose a new AsyncClient and AsyncClientBuilder

doc: Add documentation for the new async API

cd6cab6

refactor: Configure async feature flag and fix imports

9a0dc85

1c3t3a requested a review from ctrlaltf24 April 12, 2022 13:46

refactor: Change Box::pin to .boxed() in tests

31137a1

ctrlaltf24 requested changes Apr 12, 2022

View reviewed changes

1c3t3a added 2 commits April 19, 2022 18:47

refactor: Fix nits that came up during review

03cb41d

refactor: Add trace level logs and therefore the log dependency

3789e8b

1c3t3a requested a review from ctrlaltf24 April 28, 2022 21:11

ctrlaltf24 approved these changes May 17, 2022

View reviewed changes

socketio/src/asynchronous/client/client.rs Outdated Show resolved Hide resolved

ctrlaltf24 reviewed May 23, 2022

View reviewed changes

SSebo mentioned this pull request Oct 20, 2022

feat: basic async socketio server #236

Closed

1c3t3a and others added 8 commits January 15, 2023 12:57

refactor: Add an integration test and a note for the cloneable engine…

3b48185

….io client

refactor: Add a feature flag for async callbacks in both engine.io an…

d77a180

…d socket.io

refactor: Only exclude the builders on method on the async-callback…

de24bc6

…s feature

refactor: Remove unecessary unwrapping call while emitting to an event

6ff9b8e

chore: use match instead of unwrap

37c2156

fix: callback not await

36fbf71

Merge remote-tracking branch 'origin/main' into async-socketio

f14db4d

feat: Bump version to 0.4.1-alpha.1

e8032e6

1c3t3a force-pushed the async-socketio branch from 35ac8b3 to e8032e6 Compare January 15, 2023 12:21

Fix errors in README.md

185aa5b

1c3t3a merged commit b6c58c1 into main Jan 15, 2023

1c3t3a deleted the async-socketio branch March 26, 2023 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an async socket.io API #180

Add an async socket.io API #180

1c3t3a commented Apr 12, 2022

codecov bot commented Apr 12, 2022 •

edited

Loading

ctrlaltf24 left a comment

ctrlaltf24 Apr 12, 2022

1c3t3a commented Apr 19, 2022 •

edited

Loading

1c3t3a commented Apr 28, 2022

1c3t3a commented Apr 28, 2022

ctrlaltf24 commented Apr 28, 2022

ctrlaltf24 commented May 17, 2022

ctrlaltf24 May 23, 2022

JackFlukinger commented Jun 9, 2022 •

edited

Loading

SSebo commented Aug 16, 2022 •

edited

Loading

SSebo commented Aug 16, 2022 •

edited

Loading

ctrlaltf24 commented Aug 16, 2022

1c3t3a commented Jan 15, 2023

Add an async socket.io API #180

Add an async socket.io API #180

Conversation

1c3t3a commented Apr 12, 2022

codecov bot commented Apr 12, 2022 • edited Loading

Codecov Report

ctrlaltf24 left a comment

Choose a reason for hiding this comment

ctrlaltf24 Apr 12, 2022

Choose a reason for hiding this comment

1c3t3a commented Apr 19, 2022 • edited Loading

1c3t3a commented Apr 28, 2022

1c3t3a commented Apr 28, 2022

ctrlaltf24 commented Apr 28, 2022

ctrlaltf24 commented May 17, 2022

ctrlaltf24 May 23, 2022

Choose a reason for hiding this comment

JackFlukinger commented Jun 9, 2022 • edited Loading

SSebo commented Aug 16, 2022 • edited Loading

SSebo commented Aug 16, 2022 • edited Loading

ctrlaltf24 commented Aug 16, 2022

1c3t3a commented Jan 15, 2023

codecov bot commented Apr 12, 2022 •

edited

Loading

1c3t3a commented Apr 19, 2022 •

edited

Loading

JackFlukinger commented Jun 9, 2022 •

edited

Loading

SSebo commented Aug 16, 2022 •

edited

Loading

SSebo commented Aug 16, 2022 •

edited

Loading