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

sente/reconnect doesn't reset backoff-ms attempt number when chsk is closed #167

Closed
danielcompton opened this issue Sep 29, 2015 · 6 comments
Assignees
Milestone

Comments

@danielcompton
Copy link
Collaborator

From my testing, it seems like when calling sente/reconnect from a closed chsk, the backoff-ms attempt number isn't reset. The use case for this is displaying to the user the number of seconds before reconnecting, but giving them the option to reconnect faster if they want to. Currently, they can click a reconnect button, but if the channel socket is closed then nothing will happen, and Sente will try to reconnect on it's normal schedule.

Is this understanding correct, and is this a bug?

@ptaoussanis
Copy link
Member

Hey Daniel,

Your understanding's correct - though the behaviour's not technically a bug. chsk-reconnect!'s currently documented as: "Drops connection, allows auto-reconnect. Useful for reauthenticating after login/logout."

i.e. chsk-reconnect! was originally added as a quick hack as a way of dropping an existing connection to allow it to come back up. Didn't have in mind to allow it to speed up reconnect attempts of already-dropped connections, though that's a totally reasonable expectation from the name.

Would be happy for a PR to adopt the behaviour you're describing (+ update the docstring). Might not be an obvious/easy way to do this btw, so feel free to leave this to me if it's looking like it might be tricky.

Can try take a look at this myself at the weekend. Thanks for the report, the current behaviour's definitely surprising.

@danielcompton
Copy link
Collaborator Author

I'll have a look tomorrow with clearer eyes, but I think this might be part of the solution (also a note to self):

(reset! nattempt_ 0)
(.clearInterval js/window @kalive-timer_)

@ptaoussanis
Copy link
Member

but I think this might be part of the solution

The nattempt_ looks good, but note that the keep-alive timer and backoff timers are different. The backoff timer isn't currently accessible for cancelling; it's just an ephemeral thing created in set-exp-backoff-timeout!.

So we may need to add a new backoff-timer_ atom (like kalive-timer_) that we do track to allow cancelling.

@lfn3
Copy link

lfn3 commented Oct 13, 2015

So I've taken a swing at this, and what I went with is adding the connect! fn to the record and using that directly while clearing the interval timer. This means the timer gets reset but the number of attempts doesn't (which seems desirable?).

Does that sound good to you guys?

@danielcompton
Copy link
Collaborator Author

Open a PR and we can take a look :)

@ptaoussanis ptaoussanis self-assigned this Dec 8, 2015
@ptaoussanis ptaoussanis added this to the v1.8.0 milestone Dec 8, 2015
ptaoussanis added a commit that referenced this issue Jan 25, 2016
Just clean up some historical cruft to make it more obvious what's going on
@ptaoussanis
Copy link
Member

Fixed in the SNAPSHOT I'm busy uploading now...

ptaoussanis added a commit that referenced this issue Jan 25, 2016
This commit:
- Signf. simplifies the retry impln.
- Introduces the notion of retry-ids so that we can easily noop
  the effect of any pending retries just by switching out the active
  id.
- Simplified the client API: dropped `chsk-destroy!` and added
  `chsk-disconnect!`. The latter is just a reversible version of the
  latter.
ptaoussanis added a commit that referenced this issue Jan 25, 2016
This commit:
- Signf. simplifies the retry impln.
- Introduces the notion of retry-ids so that we can easily noop
  the effect of any pending retries just by switching out the active
  id.
- Simplified the client API: dropped `chsk-destroy!` and added
  `chsk-disconnect!`. The latter is just a reversible version of the
  latter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants