feat: reconnect refactor #229

SSebo · 2022-08-19T09:27:59Z

@nshaaban-cPacket This PR is based on your POC 👍
@nshaaban-cPacket @1c3t3a PTAL

codecov · 2022-08-19T09:34:01Z

Codecov Report

Merging #229 (76193b7) into main (7d5bbb4) will increase coverage by 1.06%.
The diff coverage is 89.68%.

@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   86.38%   87.44%   +1.06%     
==========================================
  Files          30       31       +1     
  Lines        2350     2478     +128     
==========================================
+ Hits         2030     2167     +137     
+ Misses        320      311       -9

Impacted Files	Coverage Δ
socketio/src/client/callback.rs	`37.50% <ø> (ø)`
socketio/src/error.rs	`100.00% <ø> (ø)`
socketio/src/socket.rs	`78.49% <66.66%> (+4.02%)`	⬆️
socketio/src/client/raw_client.rs	`87.17% <87.17%> (ø)`
socketio/src/client/client.rs	`94.91% <94.59%> (+9.05%)`	⬆️
engineio/src/client/client.rs	`94.60% <100.00%> (+0.71%)`	⬆️
engineio/src/packet.rs	`97.95% <100.00%> (ø)`
engineio/src/socket.rs	`77.77% <100.00%> (+3.70%)`	⬆️
socketio/src/client/builder.rs	`92.95% <100.00%> (+2.18%)`	⬆️
socketio/src/lib.rs	`100.00% <100.00%> (ø)`
... and 6 more

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

1c3t3a

This looks pretty good, only nits and a few minor things! Thanks for going the extra mile and adding such thorough tests, that is really good to have. The wrapper is a much cleaner solution as the one proposed in #217, thanks a lot for reacting to the feedback!

engineio/src/socket.rs

socketio/examples/callback.rs

socketio/src/client/builder.rs

socketio/src/client/reconnect.rs

SSebo · 2022-08-19T12:35:31Z

Test case is stable now by checking Open and Close callback.

ctrlaltf24

Couple blockers mostly related to maintaining public APIs. If you want to learn highly recommend this (for some reason hard to find) rust book on maintaining public APIs. The actual implementation looks much cleaner! Thanks!

socketio/src/client/builder.rs

socketio/src/client/reconnect.rs

socketio/src/socket.rs

SSebo · 2022-08-25T04:10:54Z

solution three: rename ReconnctingClient to Client (and take it's place), and rename existing client to InternalClient

@nshaaban-cPacket I tried, found that InternalClient is hard to hide from user because of the callback parameter. Compiler told me that trait is not object safe if I move emit function with generic type in.

1c3t3a

solution three: rename ReconnctingClient to Client (and take it's place), and rename existing client to InternalClient

@nshaaban-cPacket I tried, found that InternalClient is hard to hide from user because of the callback parameter. Compiler told me that trait is not object safe if I move emit function with generic type in.

Could you share the compiler message? Would be interesting to see why.

socketio/src/client/reconnect.rs

1c3t3a · 2022-08-25T10:19:23Z

socketio/src/client/reconnect.rs

+
+    fn reconnect(&mut self) -> Result<()> {
+        let mut reconnect_attempts = 0;
+        if self.builder.reconnect {


I agree with you @nshaaban-cPacket, this makes sense. Changing the Clients default reconnection behavior is a breaking change anyways, so it would make sense to release a new breaking version and be transparent about what has changed.

engineio/src/socket.rs

socketio/examples/callback.rs

SSebo · 2022-08-25T15:03:30Z

@1c3t3a Compiler does not complain until I create a trait object Box<dyn ClientTrait>:

--> socketio/src/client/reconnect.rs:40:8
|
14 | pub trait ClientTrait {
| ----------- this trait cannot be made into an object...
...
40 | fn emit<E, D>(&self, event: E, data: D) -> Result<()>
| ^^^^ ...because method emit has generic type parameters
...
80 | fn emit_with_ack<F, E, D>(
| ^^^^^^^^^^^^^ ...because method emit_with_ack has generic type parameters
= help: consider moving emit to another trait
= help: consider moving emit_with_ack to another trait

For more information about this error, try rustc --explain E0038.
error: could not compile rust_socketio due to previous error

ctrlaltf24 · 2022-08-25T18:05:56Z

If this is a breaking change anyways, we could expose BOTH ReconnectingClient and Client in the public interface

1c3t3a · 2022-08-27T18:28:31Z

@1c3t3a Compiler does not complain until I create a trait object Box<dyn ClientTrait>:

--> socketio/src/client/reconnect.rs:40:8 | 14 | pub trait ClientTrait { | ----------- this trait cannot be made into an object... ... 40 | fn emit<E, D>(&self, event: E, data: D) -> Result<()> | ^^^^ ...because method emit has generic type parameters ... 80 | fn emit_with_ack<F, E, D>( | ^^^^^^^^^^^^^ ...because method emit_with_ack has generic type parameters = help: consider moving emit to another trait = help: consider moving emit_with_ack to another trait

For more information about this error, try rustc --explain E0038. error: could not compile rust_socketio due to previous error

What is your definition of ClientTrait, and why do we necessarily need it? Anyways I agree with what @nshaaban-cPacket mentions and expose both versions of the client.

SSebo · 2022-08-29T03:20:13Z

@1c3t3a Compiler does not complain until I create a trait object Box<dyn ClientTrait>:
--> socketio/src/client/reconnect.rs:40:8 | 14 | pub trait ClientTrait { | ----------- this trait cannot be made into an object... ... 40 | fn emit<E, D>(&self, event: E, data: D) -> Result<()> | ^^^^ ...because method emit has generic type parameters ... 80 | fn emit_with_ack<F, E, D>( | ^^^^^^^^^^^^^ ...because method emit_with_ack has generic type parameters = help: consider moving emit to another trait = help: consider moving emit_with_ack to another trait
For more information about this error, try rustc --explain E0038. error: could not compile rust_socketio due to previous error

What is your definition of ClientTrait, and why do we necessarily need it? Anyways I agree with what @nshaaban-cPacket mentions and expose both versions of the client.

@1c3t3a If we expose both versions of client，we do not need this.

milandekruijf · 2022-09-12T22:56:01Z

I've been testing this for a while now and it works great. I did notice that reconnects don't happen when losing internet connection, it just simply does nothing and stops emitting events.

SSebo · 2022-09-13T04:58:08Z

I've been testing this for a while now and it works great. I did notice that reconnects don't happen when losing internet connection, it just simply does nothing and stops emitting events.

@milandekruijf @1c3t3a @nshaaban-cPacket I found the js version of engine.io client will close connection if ping time out. https://github.com/socketio/engine.io-client/blob/dfee8ded722a8c4f9f773505d0c77b4561569863/lib/socket.ts#L645. Suggest @milandekruijf to open a new issue, because the change will be in engine.io layer

TomieAi · 2022-10-11T15:52:01Z

when this feature gonna be available? pls

ctrlaltf24 · 2022-10-11T18:30:22Z

What's left to do here:

expose reconnecting client as Client in external API (returned from connect function on the ClientBuilder)
expose Client as RawClient (or something along those lines) in public API
(optional) expose second function on builder called connect_raw or something along those lines.
bump MINOR version
MERGE!

@SSebo

(Listen to @1c3t3a over me if he says anything different)

1c3t3a · 2022-10-14T14:56:51Z

Sounds good! From my experience it was never bad advice to listen to @nshaaban-cPacket :) I would accept any PR that fixes the existing questions.

SSebo · 2022-10-15T14:31:26Z

@1c3t3a @nshaaban-cPacket PTAL

1c3t3a

LGTM % comment :)

socketio/examples/callback.rs

ctrlaltf24

Blocked on removed doc comments from public interface

socketio/src/client/client.rs

ctrlaltf24 · 2022-10-17T19:04:18Z

socketio/src/client/client.rs

+pub(crate) struct Iter {
+    socket: Arc<RwLock<RawClient>>,


(will let it go because it's an internal component)
suggestion: shouldn't need locking here. Should be able to take a ref of Client, and use a poll method (that handles reconnects) similar to before.

RawClient is guarded by RwLock in Client, Can not just ref Client without hold the guard, when guard dropped, the ref is invlaid.

socketio/src/client/client.rs

socketio/src/client/raw_client.rs

socketio/src/client/client.rs

1c3t3a

Thanks a lot @SSebo for the feature! Appreciated :)

SSebo force-pushed the reconnect_refactor branch 2 times, most recently from ed777c2 to ef3f9e0 Compare August 19, 2022 09:36

SSebo changed the title ~~feat: reconnect~~ feat: reconnect refactor Aug 19, 2022

SSebo force-pushed the reconnect_refactor branch 3 times, most recently from 21c430d to d82d8f0 Compare August 19, 2022 09:47

feat: reconnect

524b61e

SSebo force-pushed the reconnect_refactor branch from d82d8f0 to 524b61e Compare August 19, 2022 10:08

1c3t3a mentioned this pull request Aug 19, 2022

feat: reconnect #217

Closed

4 tasks

1c3t3a requested changes Aug 19, 2022

View reviewed changes

SSebo force-pushed the reconnect_refactor branch 2 times, most recently from aabbcd4 to 81ceda6 Compare August 19, 2022 12:25

chore: reconnect improve readability

3624e9f

SSebo force-pushed the reconnect_refactor branch 4 times, most recently from c6864d7 to 4d5f4d0 Compare August 19, 2022 14:15

fix: reconect test case

63356cc

SSebo force-pushed the reconnect_refactor branch from 4d5f4d0 to 63356cc Compare August 19, 2022 14:38

ctrlaltf24 requested changes Aug 19, 2022

View reviewed changes

ctrlaltf24 mentioned this pull request Aug 19, 2022

feat(wip): reconnecting #228

Closed

SSebo force-pushed the reconnect_refactor branch 7 times, most recently from 698f76f to c8de969 Compare August 20, 2022 03:21

SSebo force-pushed the reconnect_refactor branch from 28266eb to e6b236b Compare August 25, 2022 04:01

1c3t3a reviewed Aug 25, 2022

View reviewed changes

chore(reconnect): test use atomic usize

8369f67

SSebo force-pushed the reconnect_refactor branch from d46678c to 8369f67 Compare August 25, 2022 15:06

SSebo requested a review from 1c3t3a August 25, 2022 15:18

SSebo force-pushed the reconnect_refactor branch from 1553876 to 71ad1e9 Compare October 15, 2022 14:12

chore: rename ReconnectClient to Client, Client to RawClient

56b651d

SSebo force-pushed the reconnect_refactor branch from 71ad1e9 to 56b651d Compare October 15, 2022 14:22

1c3t3a approved these changes Oct 17, 2022

View reviewed changes

socketio/examples/callback.rs Outdated Show resolved Hide resolved

chore: restore callback example

bc20e6c

ctrlaltf24 requested changes Oct 17, 2022

View reviewed changes

chore: add doc to client pub function

ff13ae8

ctrlaltf24 approved these changes Oct 19, 2022

View reviewed changes

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

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

chore: rename test case variable _packets to lock

76193b7

1c3t3a approved these changes Oct 20, 2022

View reviewed changes

1c3t3a merged commit f503e7a into 1c3t3a:main Oct 20, 2022

This was referenced Oct 20, 2022

Emit should not failed while reconnecting #230

Open

thread '<unnamed>' panicked at 'EngineIO Error' #216

Closed

Auto-reconnect on disconnects #96

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reconnect refactor #229

feat: reconnect refactor #229

SSebo commented Aug 19, 2022

codecov bot commented Aug 19, 2022 •

edited

Loading

1c3t3a left a comment

SSebo commented Aug 19, 2022 •

edited

Loading

ctrlaltf24 left a comment

SSebo commented Aug 25, 2022 •

edited

Loading

1c3t3a left a comment

1c3t3a Aug 25, 2022

SSebo commented Aug 25, 2022 •

edited

Loading

ctrlaltf24 commented Aug 25, 2022

1c3t3a commented Aug 27, 2022

SSebo commented Aug 29, 2022 •

edited

Loading

milandekruijf commented Sep 12, 2022

SSebo commented Sep 13, 2022

TomieAi commented Oct 11, 2022

ctrlaltf24 commented Oct 11, 2022 •

edited

Loading

1c3t3a commented Oct 14, 2022

SSebo commented Oct 15, 2022

1c3t3a left a comment

ctrlaltf24 left a comment

ctrlaltf24 Oct 17, 2022

SSebo Oct 18, 2022

1c3t3a left a comment

feat: reconnect refactor #229

feat: reconnect refactor #229

Conversation

SSebo commented Aug 19, 2022

codecov bot commented Aug 19, 2022 • edited Loading

Codecov Report

1c3t3a left a comment

Choose a reason for hiding this comment

SSebo commented Aug 19, 2022 • edited Loading

ctrlaltf24 left a comment

Choose a reason for hiding this comment

SSebo commented Aug 25, 2022 • edited Loading

1c3t3a left a comment

Choose a reason for hiding this comment

1c3t3a Aug 25, 2022

Choose a reason for hiding this comment

SSebo commented Aug 25, 2022 • edited Loading

ctrlaltf24 commented Aug 25, 2022

1c3t3a commented Aug 27, 2022

SSebo commented Aug 29, 2022 • edited Loading

milandekruijf commented Sep 12, 2022

SSebo commented Sep 13, 2022

TomieAi commented Oct 11, 2022

ctrlaltf24 commented Oct 11, 2022 • edited Loading

1c3t3a commented Oct 14, 2022

SSebo commented Oct 15, 2022

1c3t3a left a comment

Choose a reason for hiding this comment

ctrlaltf24 left a comment

Choose a reason for hiding this comment

ctrlaltf24 Oct 17, 2022

Choose a reason for hiding this comment

SSebo Oct 18, 2022

Choose a reason for hiding this comment

1c3t3a left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2022 •

edited

Loading

SSebo commented Aug 19, 2022 •

edited

Loading

SSebo commented Aug 25, 2022 •

edited

Loading

SSebo commented Aug 25, 2022 •

edited

Loading

SSebo commented Aug 29, 2022 •

edited

Loading

ctrlaltf24 commented Oct 11, 2022 •

edited

Loading