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

Stop calling onClientError when disconnect is called #194

Merged
merged 3 commits into from
Mar 28, 2017
Merged

Stop calling onClientError when disconnect is called #194

merged 3 commits into from
Mar 28, 2017

Conversation

jenow
Copy link
Contributor

@jenow jenow commented Mar 10, 2017

@jenow jenow self-assigned this Mar 10, 2017
@codecov-io
Copy link

codecov-io commented Mar 10, 2017

Codecov Report

Merging #194 into 4.x will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              4.x     #194      +/-   ##
==========================================
+ Coverage   99.36%   99.41%   +0.05%     
==========================================
  Files          16       16              
  Lines        1721     1724       +3     
  Branches      458      458              
==========================================
+ Hits         1710     1714       +4     
+ Misses         11       10       -1
Impacted Files Coverage Δ
src/networkWrapper/wrappers/websocket.js 100% <100%> (ø)
src/Kuzzle.js 99.63% <0%> (+0.18%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d2488d...1dc1716. Read the comment docs.

# Conflicts:
#	dist/kuzzle.js
#	dist/kuzzle.js.map
Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

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

I disagree with the way this PR is implemented.

To me, this bug occurs because the close method does not do its job correctly, leaving the auto-reconnection process alive instead of stopping it.

Instead of adding additional complexity to the current state machine, I propose to keep a reference to the auto-reconnection setTimeout timer, and then having the close method use it to remove the timer from the event loop.

@ballinette
Copy link

ballinette commented Mar 24, 2017

Note: the related issue will be fixed by another way when the network wrappers will be refactored.
If the bug is blocking, we can accept this quick fix.
If not, it's better to wait for the deep network wrappers refactoring.

@scottinet
Copy link
Contributor

Ok, accepting this fix for now since it'll be implemented in a cleaner way later.

@scottinet scottinet merged commit 24a8bc4 into 4.x Mar 28, 2017
@scottinet scottinet deleted the fix-16 branch March 28, 2017 09:01
This was referenced Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants