Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: connect after client.end not working #1902

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

MaximoLiberata
Copy link
Contributor

@MaximoLiberata MaximoLiberata commented Jul 16, 2024

Fixes #1897

@robertsLando robertsLando changed the title fix: connect manually fix: connect after client.end not working Jul 16, 2024
@MaximoLiberata
Copy link
Contributor Author

MaximoLiberata commented Jul 16, 2024

comment reference

@MaximoLiberata your change would solve the issue exception issue, but not that after end method finished the public disconnecting flag is still true.

@TimT1919 I think I covered that issue, check this PR. Also, the disconnecting variable is only possible to set it as false when you are the actor who call the connect method (call connect method manually), this happens because the reconnect behavior take a look for disconnecting variable before try connect again.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @MaximoLiberata I may have explained it wrongly, there is no problem if test is written using callbacks (most of tests are using callbacks), what I wanted to do is to prevent using setTimeout, them are not reliable in tests and also make test last longer (as you have to wait the timeout to trigger.

I usually fix this by not using timeouts at all but rely on events instead (when possible) or use async/await or use sinon fake timers (you can see some tests using them)

Thanks for your help 🙏🏼

@TimT1919
Copy link

@MaximoLiberata Integrated your changes to my project and could remove my workaround for this issue. So the changes seam to resolve the issue. Thanks a lot 🙏🏼

@MaximoLiberata
Copy link
Contributor Author

MaximoLiberata commented Jul 17, 2024

Sorry @MaximoLiberata I may have explained it wrongly, there is no problem if test is written using callbacks (most of tests are using callbacks), what I wanted to do is to prevent using setTimeout, them are not reliable in tests and also make test last longer (as you have to wait the timeout to trigger.

I usually fix this by not using timeouts at all but rely on events instead (when possible) or use async/await or use sinon fake timers (you can see some tests using them)

Thanks for your help 🙏🏼

You are absoluty correct, thanks. Removing setTimeout reduce time and tests won't hang up. Also, I saw if we don't close promises (async/await) that we created in the test, the test will hang up for a moment or permanent, and doesn't matter if the test it's using callback (done method), it's possible some promises didn't resolve at all. This bug happens to me when I tried throw an error with assert and I didn't close the parent promise.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.28%. Comparing base (9fc79df) to head (64304b0).
Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1902      +/-   ##
==========================================
+ Coverage   80.96%   81.28%   +0.31%     
==========================================
  Files          24       24              
  Lines        1408     1464      +56     
  Branches      331      348      +17     
==========================================
+ Hits         1140     1190      +50     
- Misses        185      188       +3     
- Partials       83       86       +3     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: disconnecting flag is not reset after Client.end is finished - Error: client disconnecting
3 participants