Skip to content

Commit

Permalink
fix for endless reconnection problem
Browse files Browse the repository at this point in the history 
* do not generate 'close' event on ping failure - rely on socket 'close' event instead
* destroy() socket instead end() to make sure 'close' event comes quickly
  • Loading branch information
@kriskep
kriskep committed May 9, 2014
1 parent 7856b18 commit 2e662a8
Showing 1 changed file with 1 addition and 2 deletions.
3 changes: 1 addition & 2 deletions lib/client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ MqttClient.prototype._clearReconnect = function() {
*/
MqttClient.prototype._cleanUp = function() {
this.conn.disconnect();
this.stream.end();
this.stream.destroy();
if (this.pingTimer !== null) {
clearInterval(this.pingTimer);
this.pingTimer = null;
Expand Down Expand Up @@ -461,7 +461,6 @@ MqttClient.prototype._checkPing = function () {
this.conn.pingreq();
} else {
this._cleanUp();
this.emit('close');
}
};

Expand Down

5 comments on commit 2e662a8

@mcollina
Copy link

Choose a reason for hiding this comment

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

This is not completely correct, as calling this.conn.disconnect() sends a MQTT disconnect message, that must be sent in case of normal close (e.g. through end). calling this.stream.destroy() will prevent that message to be sent, while this.stream.end() ensure that.

You can get around that problem by adding a parameter to cleanup.

.. and please, add tests!

@kriskep
Copy link
Owner Author

@kriskep kriskep commented on 2e662a8 May 9, 2014

Choose a reason for hiding this comment

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

You are right. I wasn't too sure about using destroy() here.

I think we may even leave this.stream.end() as it was, however, we have to keep in mind it may take a long time before the actual close event is generated in case of dropped network. During that time, message are being lost because there is no flag set to indicate the connection is disconnecting.

@kriskep
Copy link
Owner Author

@kriskep kriskep commented on 2e662a8 May 9, 2014

Choose a reason for hiding this comment

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

a better fix here 1090fa6

I'd be happy to submit tests, but how do you actually run these tests?

@mcollina
Copy link

Choose a reason for hiding this comment

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

the tests are written in mocha. You can run them by issuing npm test or mocha

@kriskep
Copy link
Owner Author

@kriskep kriskep commented on 2e662a8 May 9, 2014

Choose a reason for hiding this comment

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

It may be difficult to include it in an automated testsuite because my test requires root access to modify iptables. See mqttjs#180 for code.
Just stopping the server doesn't do it because that will close the socket as well.

Please sign in to comment.