From 3da5e84a158985cbe7bdf60d3a9744b71d98bb56 Mon Sep 17 00:00:00 2001
From: Daniel Lando <daniel.sorridi@gmail.com>
Date: Tue, 23 Jan 2024 15:09:06 +0100
Subject: [PATCH] fix: keepalive causes a reconnect loop when connection is
 lost (#1779)

* fix: keepalive causes a reconnect loop when connection is lost

Fixes #1778

* fix: add test
---
 example.ts           | 47 +++++++++++++++++++++++++++++++++------
 src/lib/PingTimer.ts | 18 +++++++--------
 src/lib/client.ts    |  8 +++----
 test/pingTimer.ts    | 53 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 20 deletions(-)
 create mode 100644 test/pingTimer.ts

diff --git a/example.ts b/example.ts
index 6800abad4..1f26a4b6a 100644
--- a/example.ts
+++ b/example.ts
@@ -1,19 +1,29 @@
 import mqtt from '.'
 
-const client = mqtt.connect('mqtt://test.mosquitto.org')
+const client = mqtt.connect('mqtt://test.mosquitto.org', {
+	keepalive: 10,
+	reconnectPeriod: 15000,
+})
 
 const testTopic = 'presence'
 
-client.subscribe(testTopic, (err) => {
-	if (!err) {
-		console.log('subscribed to', testTopic)
-		client.publish(testTopic, 'Hello mqtt', (err2) => {
+function publish() {
+	client.publish(
+		testTopic,
+		`Hello mqtt ${new Date().toISOString()}`,
+		(err2) => {
 			if (!err2) {
 				console.log('message published')
 			} else {
 				console.error(err2)
 			}
-		})
+		},
+	)
+}
+
+client.subscribe(testTopic, (err) => {
+	if (!err) {
+		console.log('subscribed to', testTopic)
 	} else {
 		console.error(err)
 	}
@@ -21,5 +31,28 @@ client.subscribe(testTopic, (err) => {
 
 client.on('message', (topic, message) => {
 	console.log('received message "%s" from topic "%s"', message, topic)
-	client.end()
+	setTimeout(() => {
+		publish()
+	}, 2000)
+})
+
+client.on('error', (err) => {
+	console.error(err)
+})
+
+client.on('connect', () => {
+	console.log('connected')
+	publish()
+})
+
+client.on('disconnect', () => {
+	console.log('disconnected')
+})
+
+client.on('offline', () => {
+	console.log('offline')
+})
+
+client.on('reconnect', () => {
+	console.log('reconnect')
 })
diff --git a/src/lib/PingTimer.ts b/src/lib/PingTimer.ts
index 6f601b5b5..ced451fa2 100644
--- a/src/lib/PingTimer.ts
+++ b/src/lib/PingTimer.ts
@@ -21,14 +21,7 @@ export default class PingTimer {
 	constructor(keepalive: number, checkPing: () => void) {
 		this.keepalive = keepalive * 1000
 		this.checkPing = checkPing
-		this.setup()
-	}
-
-	private setup() {
-		this.timer = this._setTimeout(() => {
-			this.checkPing()
-			this.reschedule()
-		}, this.keepalive)
+		this.reschedule()
 	}
 
 	clear() {
@@ -40,6 +33,13 @@ export default class PingTimer {
 
 	reschedule() {
 		this.clear()
-		this.setup()
+		this.timer = this._setTimeout(() => {
+			this.checkPing()
+			// prevent possible race condition where the timer is destroyed on _cleauUp
+			// and recreated here
+			if (this.timer) {
+				this.reschedule()
+			}
+		}, this.keepalive)
 	}
 }
diff --git a/src/lib/client.ts b/src/lib/client.ts
index 8e1be4e3b..f7a9b4dcc 100644
--- a/src/lib/client.ts
+++ b/src/lib/client.ts
@@ -639,7 +639,7 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
 			clearTimeout(this.connackTimer)
 
 			this.log('close :: clearing ping timer')
-			if (this.pingTimer !== null) {
+			if (this.pingTimer) {
 				this.pingTimer.clear()
 				this.pingTimer = null
 			}
@@ -1752,15 +1752,15 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
 			})
 		}
 
-		if (!this.disconnecting) {
+		if (!this.disconnecting && !this.reconnecting) {
 			this.log(
-				'_cleanUp :: client not disconnecting. Clearing and resetting reconnect.',
+				'_cleanUp :: client not disconnecting/reconnecting. Clearing and resetting reconnect.',
 			)
 			this._clearReconnect()
 			this._setupReconnect()
 		}
 
-		if (this.pingTimer !== null) {
+		if (this.pingTimer) {
 			this.log('_cleanUp :: clearing pingTimer')
 			this.pingTimer.clear()
 			this.pingTimer = null
diff --git a/test/pingTimer.ts b/test/pingTimer.ts
new file mode 100644
index 000000000..28d7d650d
--- /dev/null
+++ b/test/pingTimer.ts
@@ -0,0 +1,53 @@
+import { afterEach, beforeEach, describe, it } from 'node:test'
+import PingTimer from '../src/lib/PingTimer'
+import { assert } from 'chai'
+import { useFakeTimers, spy } from 'sinon'
+
+describe('PingTimer', () => {
+	let clock: sinon.SinonFakeTimers
+	beforeEach(() => {
+		clock = useFakeTimers()
+	})
+
+	afterEach(() => {
+		clock.restore()
+	})
+
+	it('should schedule and clear', () => {
+		const keepalive = 10 // seconds
+		const cb = spy()
+		const pingTimer = new PingTimer(keepalive, cb)
+
+		assert.ok(pingTimer['timer'], 'timer should be created automatically')
+
+		clock.tick(keepalive * 1000 + 1)
+		assert.equal(
+			cb.callCount,
+			1,
+			'should trigger the callback after keepalive seconds',
+		)
+		clock.tick(keepalive * 1000 + 1)
+		assert.equal(cb.callCount, 2, 'should reschedule automatically')
+		pingTimer.clear()
+		assert.ok(!pingTimer['timer'], 'timer should not exists after clear()')
+	})
+
+	it('should not re-schedule if timer has been cleared in check ping', () => {
+		const keepalive = 10 // seconds
+		const cb = spy()
+		const pingTimer = new PingTimer(keepalive, () => {
+			pingTimer.clear()
+			cb()
+		})
+
+		clock.tick(keepalive * 1000 + 1)
+		assert.equal(
+			cb.callCount,
+			1,
+			'should trigger the callback after keepalive seconds',
+		)
+		clock.tick(keepalive * 1000 + 1)
+		assert.equal(cb.callCount, 1, 'should not re-schedule')
+		assert.ok(!pingTimer['timer'], 'timer should not exists')
+	})
+})