fix: possible race condition in ping timer (#1848)

Ref: #1845

Author: robertsLando

package-lock.json

@@ -10,6 +10,8 @@ export default class PingTimer {
 
 	private checkPing: () => void
 
+	private destroyed = false
+
 	constructor(
 		keepalive: number,
 		checkPing: () => void,
@@ -21,22 +23,34 @@ export default class PingTimer {
 		this.reschedule()
 	}
 
-	clear() {
+	private clear() {
 		if (this.timerId) {
 			this.timer.clear(this.timerId)
 			this.timerId = null
 		}
 	}
 
+	destroy() {
+		this.clear()
+		this.destroyed = true
+	}
+
 	reschedule() {
+		if (this.destroyed) {
+			return
+		}
+
 		this.clear()
 		this.timerId = this.timer.set(() => {
-			this.checkPing()
-			// prevent possible race condition where the timer is destroyed on _cleauUp
-			// and recreated here
-			if (this.timerId) {
-				this.reschedule()
+			// this should never happen, but just in case
+			if (this.destroyed) {
+				return
 			}
+
+			this.checkPing()
+			// this must be called after `checkPing` otherwise in case `destroy`
+			// is called in `checkPing` the timer would be rescheduled anyway
+			this.reschedule()
 		}, this.keepalive)
 	}
 }

src/lib/PingTimer.ts

@@ -659,9 +659,9 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
 			this.log('close :: clearing connackTimer')
 			clearTimeout(this.connackTimer)
 
-			this.log('close :: clearing ping timer')
+			this.log('close :: destroy ping timer')
 			if (this.pingTimer) {
-				this.pingTimer.clear()
+				this.pingTimer.destroy()
 				this.pingTimer = null
 			}
 
@@ -1783,8 +1783,8 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
 		}
 
 		if (this.pingTimer) {
-			this.log('_cleanUp :: clearing pingTimer')
-			this.pingTimer.clear()
+			this.log('_cleanUp :: destroy pingTimer')
+			this.pingTimer.destroy()
 			this.pingTimer = null
 		}
 

src/lib/client.ts

@@ -13,7 +13,7 @@ describe('PingTimer', () => {
 		clock.restore()
 	})
 
-	it('should schedule and clear', () => {
+	it('should schedule and destroy', () => {
 		const keepalive = 10 // seconds
 		const cb = spy()
 		const pingTimer = new PingTimer(keepalive, cb, 'auto')
@@ -28,10 +28,15 @@ describe('PingTimer', () => {
 		)
 		clock.tick(keepalive * 1000 + 1)
 		assert.equal(cb.callCount, 2, 'should reschedule automatically')
-		pingTimer.clear()
+		pingTimer.destroy()
 		assert.ok(
 			!pingTimer['timerId'],
-			'timer should not exists after clear()',
+			'timer should not exists after destroy()',
+		)
+
+		assert.ok(
+			pingTimer['destroyed'],
+			'timer should have `destroyed` set to true after destroy()',
 		)
 	})
 
@@ -41,7 +46,7 @@ describe('PingTimer', () => {
 		const pingTimer = new PingTimer(
 			keepalive,
 			() => {
-				pingTimer.clear()
+				pingTimer.destroy()
 				cb()
 			},
 			'auto',