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

Immediately reconnect when called on with closed socket. #174

Closed

Conversation

lfn3
Copy link

@lfn3 lfn3 commented Oct 18, 2015

This makes chsk-reconnect!, when called on a closed socket,
attempt to connect immediately. It retains the original behaviour
of incrementing the backoff.

There is one issue with this at present, which is that it can cause
multiple sockets to be opened (if chsk-reconnect! is used repeatedly,
or if the timer fires at exactly the same time, probably.)

Considering adding a mutex "connecting?" to the state atom, but wanted
to check that was sane first.

ptaoussanis and others added 2 commits September 28, 2015 19:29
This makes chsk-reconnect!, when called on a closed socket,
attempt to connect immediately. It retains the original behaviour
of incrementing the backoff.

There is one issue with this at present, which is that it can cause
multiple sockets to be opened (if chsk-reconnect! is used repeatedly,
or if the timer fires at exactly the same time, probably.)

Considering adding a mutex "connecting?" to the state atom, but wanted
to check that was sane first.
@lfn3
Copy link
Author

lfn3 commented Oct 18, 2015

This should fix #167, @danielcompton

@danielcompton
Copy link
Collaborator

I'd probably expect that forcing a reconnect would restart the backoff count from 0 again. What do you think?

@lfn3
Copy link
Author

lfn3 commented Oct 18, 2015

I don't really have strong feelings either way. I've thrown that in.

Any thoughts on the reconnect mutex-y thing? Is there a better way of doing that than adding another bit to the state atom?

@ptaoussanis
Copy link
Member

Hi there,

Sorry for not taking a look at this sooner. Will definitely get this sorted out for the next release (v1.8.0).

@ptaoussanis
Copy link
Member

Hi Liam,

Sorry for never getting this merged. Have taken a look and think it'd be better long-term to take a moment to properly refactor the reconnection logic to better support this use case. The current implementation is a big of a kludge.

Your PR definitely helped move this along so again, thank you! Going to keep the original issue (#167) open until I have a fix up.

Cheers :-)

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.

3 participants