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

Ensure client attempts a reconnect on no PINGRESP #146

Merged
merged 1 commit into from
Nov 27, 2013

Conversation

keithamus
Copy link
Contributor

Fixes #144

On every PINGREQ it sets this.pingResp to false, then if this.pingResp is still false by the next pingreq interval, it resets the connection. this.pingResp is set to true for the first ping.

Then, there is a handler on PINGRESPs which simply sets this.pingResp to true, so that a successful pingresp can be noted.

I'm not 100% happy with the tests - while they do test the functionality they're very "side-effecty".

@mcollina
Copy link
Member

the build is failing on node v0.8, could you please fix it?

(I know 0.8 is old, but we still support it).

@keithamus
Copy link
Contributor Author

I hate to say it but the tests are not consistently passing on my machine on origin/master or on my build, however they do pass. I think there might be some race conditions inside the test suite

Travis will pass if you re-run it on this branch, I believe.

@keithamus
Copy link
Contributor Author

Build is passing now (I didn't change any code, I just amended the commit and force pushed until the build went green)

@adamvr
Copy link
Member

adamvr commented Nov 26, 2013

Yeah, the timer dependent stuff really sucks, particularly on travis.

On calling _reconnect: I think a better idea is to kill the connection and let reconnect kick in automatically, so that reconnect options are respected.

@mcollina
Copy link
Member

I think we should move all the timers to use sinon fake timers, they work great in simulating the passing of time.
Maybe that's another pull-request.

BTW, having fixed timeouts sucks. You really want random exponential backoff, as it ensure not all 10.000 clients reconnects at the very same time, crashing you again and again. (another pull request).
For this reason, I think that it's better to let reconnect kick in automatically.

@mcollina
Copy link
Member

I'm merging this in and removing the call to _reconnect().

@mcollina mcollina merged commit 61165c3 into mqttjs:master Nov 27, 2013
@keithamus keithamus deleted the reconnect-on-no-pingresp branch November 27, 2013 12:37
@mcollina
Copy link
Member

@adamvr I'm releasing this and #143 as 0.3.5, if it's ok for you.

@adamvr
Copy link
Member

adamvr commented Nov 27, 2013

👍 Fine by me

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.

MQTT Client does not timeout after a PINGREQ
3 participants