Async reconnections #400

rageshkrishna · 2024-03-18T11:32:29Z

The async client and builder can now attempt to reconnect to the server on connection errors.

Fixes #291

1c3t3a

Thanks a lot for the PR! Really appreciate you working on that :) I left a few comments, generally this looks good.

socketio/src/asynchronous/client/builder.rs

socketio/src/asynchronous/client/client.rs

rageshkrishna · 2024-03-19T08:01:18Z

@1c3t3a I've incorporated all the suggestions. While adding the test I realized I hadn't made Client::socket an Arc. This is necessary because the reconnect thread is working on a clone and we need to be able to share the inner socket instance with all the clones. That's the only big change other than what's in the PR comments.

I made quite a few commits while trying to fix that, but I've squashed everything into just one so it's a clean update. Please take another look and let me know if I've missed something. Thanks!

rageshkrishna · 2024-03-19T08:06:14Z

There's one more problem that I really think should be solved with reconnects. The async client today has no way to update the auth field once it's set. This is a problem because, in my case, I'm using short-lived tokens to authenticate the connection. It's very unlikely that this token will be valid when the time comes to reconnect, so we need some way for the user to provide new auth data.

This might be beyond the scope of this PR, though. Maybe I should open another issue for it? Let me know what you think.

1c3t3a

Looks good!

I hadn't made Client::socket an Arc

Yes, that's one necessary step!

auth field

Maybe better to do this separately! :)

socketio/src/asynchronous/client/client.rs

Colin1860

Hi, @1c3t3a asked me to take a look at this. These are my first thoughts.

socketio/src/asynchronous/client/client.rs

1c3t3a · 2024-03-21T09:17:13Z

Lets run the CI!

1c3t3a · 2024-03-21T18:55:31Z

I fixed the tokio CI issue in main, feel free to rebase :) Then we see the "real" tests of this PR.

The builder can now be configured with the following properties to control automatic reconnections on network errors or server disconnects: - reconnect - reconnect_on_disconnect - reconnect_delay (min and max) - max_reconnect_attempts

The async client can now be configured to automatically reconnect on network errors or server disconnections

The enum replaces the need for multiple `AtomicBool`'s to maintain the disconnection reason. This makes the code easier to read and more ergonomic to maintain the state.

rageshkrishna · 2024-03-22T07:22:53Z

Thanks @1c3t3a. Rebased just now. Let's see how it goes! 😄

rageshkrishna · 2024-03-22T08:34:07Z

Well, that was dumb of me. We can't do a blocking_read inside as_stream because it blocks the runtime. I've changed as_stream to be async. I've pushed the update now with all tests passing locally.

I somehow forgot to run the tests after making that change yesterday. Sorry about that!

1c3t3a · 2024-03-22T09:27:00Z

socketio/src/asynchronous/client/client.rs

@@ -854,7 +847,7 @@ mod test {
            .await?;

        // open packet
-        let mut socket_stream = socket.as_stream();
+        let mut socket_stream = socket.as_stream().await;
        let _ = socket_stream.next().await.unwrap()?;

        println!("Here12");


Just saw this now: Where does that come from? :) (The println)

It wasn't me. Apparently you added it 12 months ago: https://github.com/1c3t3a/rust-socketio/pull/296/files#diff-dd5d6ff18628a21723847133d6e54ab531454037b62be81d2c42109d333aeab9R609 😆

I'll remove it.

rageshkrishna · 2024-03-22T09:37:20Z

Is the current failure a flaky test? It's coming from a sync client test, but I'm getting 100% pass locally.

1c3t3a · 2024-03-22T10:36:57Z

Is the current failure a flaky test?

I never saw a flaky test in this repo.. that is weird. How do you execute the servers locally? Do you use the devcontainer?

1c3t3a · 2024-03-22T10:45:56Z

Does the server side test have any state? E.g. you first execute the async test and then it doesn't work anymore?
Can you try executing both tests after each other locally?

rageshkrishna · 2024-03-22T10:46:46Z

I never saw a flaky test in this repo.. that is weird. How do you execute the servers locally? Do you use the devcontainer?

No, I fired up the docker container for the websocket server and ran cargo test like it's written in the ci readme.

rageshkrishna · 2024-03-22T11:01:22Z

Ahh, I see what I was missing now... I wasn't passing --features async to the test. I'm able to repro this failure on my branch. I'll try to get to the bottom of this.

Tests that rely on the reconnect socket server cannot run in parallel because they ask the server to disconnect and restart as part of the test. This can cause other tests running in parallel to fail in mysterious ways. By using the `serial_test` module and applying it selectively to the affected tests, we can ensure that they are never executed concurrently.

…socketio into feat/async-reconnect

rageshkrishna · 2024-03-22T11:33:08Z

It was because the new test I added in the async client uses the same crate::test::socket_io_restart_server() that the existing test in the sync client uses. Since these tests execute in parallel, they clobber each other's expectations of when the server will connect/disconnect their connections.

My solution to this is adding a new dev-dependency on the serial_test crate, and then marking both of these reconnect tests with the [serial_test(reconnect)] attribute. reconnect is just a key to identify which tests must be executed serially, so this doesn't affect the other tests in any way.

With this, cargo test --features async is all green. Please take a look and let me know what you think.

rageshkrishna · 2024-03-22T11:34:44Z

Here's the commit that adds it: e2eea62

codecov · 2024-03-22T17:33:14Z

Codecov Report

Attention: Patch coverage is 91.09948% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 92.05%. Comparing base (4324d7b) to head (cb7311f).

Files	Patch %	Lines
socketio/src/asynchronous/client/client.rs	91.61%	13 Missing ⚠️
socketio/src/asynchronous/client/builder.rs	88.57%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #400      +/-   ##
==========================================
+ Coverage   91.97%   92.05%   +0.07%     
==========================================
  Files          36       36              
  Lines        4872     5007     +135     
==========================================
+ Hits         4481     4609     +128     
- Misses        391      398       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

1c3t3a

LGTM! This was a big one, thanks a lot @rageshkrishna for implementing this, really appreciated :)

1c3t3a reviewed Mar 18, 2024

View reviewed changes

rageshkrishna force-pushed the feat/async-reconnect branch 3 times, most recently from afb2e91 to fab1dab Compare March 19, 2024 07:58

rageshkrishna marked this pull request as ready for review March 19, 2024 08:03

1c3t3a reviewed Mar 19, 2024

View reviewed changes

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

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

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

rageshkrishna force-pushed the feat/async-reconnect branch 2 times, most recently from 167edd4 to 3dcf5ee Compare March 19, 2024 10:16

Colin1860 reviewed Mar 20, 2024

View reviewed changes

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

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

rageshkrishna added 3 commits March 22, 2024 12:51

Add reconnect settings to async builder

bf02c60

The builder can now be configured with the following properties to control automatic reconnections on network errors or server disconnects: - reconnect - reconnect_on_disconnect - reconnect_delay (min and max) - max_reconnect_attempts

feat(async):Async reconnections

795cdbd

The async client can now be configured to automatically reconnect on network errors or server disconnections

Introduce DisconnectReason enum

210fe95

The enum replaces the need for multiple `AtomicBool`'s to maintain the disconnection reason. This makes the code easier to read and more ergonomic to maintain the state.

rageshkrishna force-pushed the feat/async-reconnect branch from 20243af to 210fe95 Compare March 22, 2024 07:22

Make as_stream async and remove blocking read

ac1ad92

1c3t3a reviewed Mar 22, 2024

View reviewed changes

Remove unnecessary println in test

1db90db

rageshkrishna added 2 commits March 22, 2024 16:57

Merge branch 'feat/async-reconnect' of github.com:rageshkrishna/rust-…

cb7311f

…socketio into feat/async-reconnect

1c3t3a approved these changes Mar 22, 2024

View reviewed changes

1c3t3a merged commit 5ae3d4d into 1c3t3a:main Mar 22, 2024
4 checks passed

SenseiHiraku mentioned this pull request Sep 23, 2024

fix: manual disconnect not preventing reconnection #466

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async reconnections #400

Async reconnections #400

rageshkrishna commented Mar 18, 2024 •

edited

Loading

1c3t3a left a comment

rageshkrishna commented Mar 19, 2024

rageshkrishna commented Mar 19, 2024

1c3t3a left a comment

Colin1860 left a comment

1c3t3a commented Mar 21, 2024

1c3t3a commented Mar 21, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

1c3t3a Mar 22, 2024 •

edited

Loading

rageshkrishna Mar 22, 2024

rageshkrishna Mar 22, 2024

rageshkrishna commented Mar 22, 2024

1c3t3a commented Mar 22, 2024

1c3t3a commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

codecov bot commented Mar 22, 2024

1c3t3a left a comment

Async reconnections #400

Async reconnections #400

Conversation

rageshkrishna commented Mar 18, 2024 • edited Loading

1c3t3a left a comment

Choose a reason for hiding this comment

rageshkrishna commented Mar 19, 2024

rageshkrishna commented Mar 19, 2024

1c3t3a left a comment

Choose a reason for hiding this comment

Colin1860 left a comment

Choose a reason for hiding this comment

1c3t3a commented Mar 21, 2024

1c3t3a commented Mar 21, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

1c3t3a Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

rageshkrishna Mar 22, 2024

Choose a reason for hiding this comment

rageshkrishna Mar 22, 2024

Choose a reason for hiding this comment

rageshkrishna commented Mar 22, 2024

1c3t3a commented Mar 22, 2024

1c3t3a commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

rageshkrishna commented Mar 22, 2024

codecov bot commented Mar 22, 2024

Codecov Report

1c3t3a left a comment

Choose a reason for hiding this comment

rageshkrishna commented Mar 18, 2024 •

edited

Loading

1c3t3a Mar 22, 2024 •

edited

Loading