feat: timerVariant option to choose between native and worker timers (#1818)

* feat: option to choose between native and worker timers

* feat: initialize timer once

* fix: pingTimer test check correct timerId prop

---------

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>

Author: someengineer-nordsec

DEVELOPMENT.md

@@ -19,6 +19,11 @@ npm test
 
 This will run both `browser` and `node` tests.
 
+
+### Running specific tests
+
+For example, you can run `node -r esbuild-register --test test/pingTimer.ts`
+
 ### Browser
 
 Browser tests use [`wtr`](https://modern-web.dev/docs/test-runner/overview/) as the test runner. To build browser bundle using [esbuild](https://esbuild.github.io/) and run browser tests, you can use the following command:

README.md

@@ -457,6 +457,7 @@ The arguments are:
   - `messageIdProvider`: custom messageId provider. when `new UniqueMessageIdProvider()` is set, then non conflict messageId is provided.
   - `log`: custom log function. Default uses [debug](https://www.npmjs.com/package/debug) package.
   - `manualConnect`: prevents the constructor to call `connect`. In this case after the `mqtt.connect` is called you should call `client.connect` manually.
+  - `timerVariant`: defaults to `auto`, which tries to determine which timer is most appropriate for you environment, if you're having detection issues, you can set it to `worker` or `native`
 
 In case mqtts (mqtt over tls) is required, the `options` object is passed through to [`tls.connect()`](http://nodejs.org/api/tls.html#tls_tls_connect_options_callback). If using a **self-signed certificate**, set `rejectUnauthorized: false`. However, be cautious as this exposes you to potential man in the middle attacks and isn't recommended for production.
 

src/lib/PingTimer.ts

@@ -1,32 +1,40 @@
-import timers from './timers'
+import getTimer, { type Timer } from './get-timer'
+import type { TimerVariant } from './shared'
 
 export default class PingTimer {
 	private keepalive: number
 
-	private timer: any
+	private timerId: number
+
+	private timer: Timer
 
 	private checkPing: () => void
 
-	constructor(keepalive: number, checkPing: () => void) {
+	constructor(
+		keepalive: number,
+		checkPing: () => void,
+		variant: TimerVariant,
+	) {
 		this.keepalive = keepalive * 1000
 		this.checkPing = checkPing
+		this.timer = getTimer(variant)
 		this.reschedule()
 	}
 
 	clear() {
-		if (this.timer) {
-			timers.clear(this.timer)
-			this.timer = null
+		if (this.timerId) {
+			this.timer.clear(this.timerId)
+			this.timerId = null
 		}
 	}
 
 	reschedule() {
 		this.clear()
-		this.timer = timers.set(() => {
+		this.timerId = this.timer.set(() => {
 			this.checkPing()
 			// prevent possible race condition where the timer is destroyed on _cleauUp
 			// and recreated here
-			if (this.timer) {
+			if (this.timerId) {
 				this.reschedule()
 			}
 		}, this.keepalive)

src/lib/client.ts

@@ -32,6 +32,7 @@ import {
 	GenericCallback,
 	IStream,
 	StreamBuilder,
+	TimerVariant,
 	VoidCallback,
 	nextTick,
 } from './shared'
@@ -49,7 +50,7 @@ const setImmediate =
 		})
 	}) as typeof globalThis.setImmediate)
 
-const defaultConnectOptions = {
+const defaultConnectOptions: IClientOptions = {
 	keepalive: 60,
 	reschedulePings: true,
 	protocolId: 'MQTT',
@@ -59,6 +60,7 @@ const defaultConnectOptions = {
 	clean: true,
 	resubscribe: true,
 	writeCache: true,
+	timerVariant: 'auto',
 }
 
 export type MqttProtocol =
@@ -266,6 +268,10 @@ export interface IClientOptions extends ISecureClientOptions {
 	will?: IConnectPacket['will']
 	/** see `connect` packet: https://github.com/mqttjs/mqtt-packet/blob/master/types/index.d.ts#L65 */
 	properties?: IConnectPacket['properties']
+	/**
+	 * @description 'auto', set to 'native' or 'worker' if you're having issues with 'auto' detection
+	 */
+	timerVariant?: TimerVariant
 }
 
 export interface IClientPublishOptions {
@@ -2078,9 +2084,13 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac
 
 		if (!this.pingTimer && this.options.keepalive) {
 			this.pingResp = true
-			this.pingTimer = new PingTimer(this.options.keepalive, () => {
-				this._checkPing()
-			})
+			this.pingTimer = new PingTimer(
+				this.options.keepalive,
+				() => {
+					this._checkPing()
+				},
+				this.options.timerVariant,
+			)
 		}
 	}
 

src/lib/get-timer.ts

@@ -0,0 +1,38 @@
+import isBrowser, { isWebWorker } from './is-browser'
+import { clearTimeout as clearT, setTimeout as setT } from 'worker-timers'
+import type { TimerVariant } from './shared'
+
+// dont directly assign globals to class props otherwise this throws in web workers: Uncaught TypeError: Illegal invocation
+// See: https://stackoverflow.com/questions/9677985/uncaught-typeerror-illegal-invocation-in-chrome
+
+export interface Timer {
+	set: typeof setT
+	clear: typeof clearT
+}
+
+const workerTimer: Timer = {
+	set: setT,
+	clear: clearT,
+}
+
+const nativeTimer: Timer = {
+	set: (func, time) => setTimeout(func, time),
+	clear: (timerId) => clearTimeout(timerId),
+}
+
+const getTimer = (variant: TimerVariant): Timer => {
+	switch (variant) {
+		case 'native': {
+			return nativeTimer
+		}
+		case 'worker': {
+			return workerTimer
+		}
+		case 'auto':
+		default: {
+			return isBrowser && !isWebWorker ? workerTimer : nativeTimer
+		}
+	}
+}
+
+export default getTimer

src/lib/shared.ts

@@ -27,6 +27,8 @@ export type PacketHandler = (
 	done?: DoneCallback,
 ) => void
 
+export type TimerVariant = 'auto' | 'worker' | 'native'
+
 export class ErrorWithReasonCode extends Error {
 	public code: number
 

src/lib/timers.ts

@@ -16,9 +16,9 @@ describe('PingTimer', () => {
 	it('should schedule and clear', () => {
 		const keepalive = 10 // seconds
 		const cb = spy()
-		const pingTimer = new PingTimer(keepalive, cb)
+		const pingTimer = new PingTimer(keepalive, cb, 'auto')
 
-		assert.ok(pingTimer['timer'], 'timer should be created automatically')
+		assert.ok(pingTimer['timerId'], 'timer should be created automatically')
 
 		clock.tick(keepalive * 1000 + 1)
 		assert.equal(
@@ -29,16 +29,23 @@ describe('PingTimer', () => {
 		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()')
+		assert.ok(
+			!pingTimer['timerId'],
+			'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()
-		})
+		const pingTimer = new PingTimer(
+			keepalive,
+			() => {
+				pingTimer.clear()
+				cb()
+			},
+			'auto',
+		)
 
 		clock.tick(keepalive * 1000 + 1)
 		assert.equal(
@@ -48,6 +55,6 @@ describe('PingTimer', () => {
 		)
 		clock.tick(keepalive * 1000 + 1)
 		assert.equal(cb.callCount, 1, 'should not re-schedule')
-		assert.ok(!pingTimer['timer'], 'timer should not exists')
+		assert.ok(!pingTimer['timerId'], 'timer should not exists')
 	})
 })