From 9e956aaae1c34629ee3107254c617309f56ca26c Mon Sep 17 00:00:00 2001 From: olso Date: Fri, 15 Mar 2024 11:04:26 +0100 Subject: [PATCH 1/3] feat: option to choose between native and worker timers --- README.md | 1 + src/lib/PingTimer.ts | 16 ++++++++++++---- src/lib/client.ts | 18 ++++++++++++++---- src/lib/get-timer.ts | 38 ++++++++++++++++++++++++++++++++++++++ src/lib/shared.ts | 2 ++ src/lib/timers.ts | 15 --------------- test/pingTimer.ts | 14 +++++++++----- 7 files changed, 76 insertions(+), 28 deletions(-) create mode 100644 src/lib/get-timer.ts delete mode 100644 src/lib/timers.ts diff --git a/README.md b/README.md index 44748d1b9..5b70a294d 100644 --- a/README.md +++ b/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. diff --git a/src/lib/PingTimer.ts b/src/lib/PingTimer.ts index f5c2fbdb6..0ffbcab0e 100644 --- a/src/lib/PingTimer.ts +++ b/src/lib/PingTimer.ts @@ -1,28 +1,36 @@ -import timers from './timers' +import getTimer from './get-timer' +import type { TimerVariant } from './shared' export default class PingTimer { private keepalive: number private timer: any + private variant: TimerVariant + private checkPing: () => void - constructor(keepalive: number, checkPing: () => void) { + constructor( + keepalive: number, + checkPing: () => void, + variant: TimerVariant, + ) { this.keepalive = keepalive * 1000 this.checkPing = checkPing + this.variant = variant this.reschedule() } clear() { if (this.timer) { - timers.clear(this.timer) + getTimer(this.variant).clear(this.timer) this.timer = null } } reschedule() { this.clear() - this.timer = timers.set(() => { + this.timer = getTimer(this.variant).set(() => { this.checkPing() // prevent possible race condition where the timer is destroyed on _cleauUp // and recreated here diff --git a/src/lib/client.ts b/src/lib/client.ts index fe299460d..fd2701faa 100644 --- a/src/lib/client.ts +++ b/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 { @@ -2076,9 +2082,13 @@ export default class MqttClient extends TypedEventEmitter { - this._checkPing() - }) + this.pingTimer = new PingTimer( + this.options.keepalive, + () => { + this._checkPing() + }, + this.options.timerVariant, + ) } } diff --git a/src/lib/get-timer.ts b/src/lib/get-timer.ts new file mode 100644 index 000000000..82d58cc3f --- /dev/null +++ b/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 + +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 diff --git a/src/lib/shared.ts b/src/lib/shared.ts index 949202375..bdbee8c43 100644 --- a/src/lib/shared.ts +++ b/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 diff --git a/src/lib/timers.ts b/src/lib/timers.ts deleted file mode 100644 index 008830994..000000000 --- a/src/lib/timers.ts +++ /dev/null @@ -1,15 +0,0 @@ -import isBrowser, { isWebWorker } from './is-browser' -import { clearTimeout as clearT, setTimeout as setT } from 'worker-timers' - -// 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 - -const timers: { set: typeof setT; clear: typeof clearT } = { - set: - isBrowser && !isWebWorker - ? setT - : (func, time) => setTimeout(func, time), - clear: isBrowser && !isWebWorker ? clearT : (timer) => clearTimeout(timer), -} - -export default timers diff --git a/test/pingTimer.ts b/test/pingTimer.ts index 28d7d650d..9cf0b2716 100644 --- a/test/pingTimer.ts +++ b/test/pingTimer.ts @@ -16,7 +16,7 @@ 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') @@ -35,10 +35,14 @@ describe('PingTimer', () => { 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( From ded0fcec8b6cde73f1b386d3bca6739ada934b9d Mon Sep 17 00:00:00 2001 From: olso Date: Fri, 15 Mar 2024 11:21:48 +0100 Subject: [PATCH 2/3] feat: initialize timer once --- src/lib/PingTimer.ts | 18 +++++++++--------- src/lib/get-timer.ts | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lib/PingTimer.ts b/src/lib/PingTimer.ts index 0ffbcab0e..e5c7b9f09 100644 --- a/src/lib/PingTimer.ts +++ b/src/lib/PingTimer.ts @@ -1,12 +1,12 @@ -import getTimer from './get-timer' +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 variant: TimerVariant + private timer: Timer private checkPing: () => void @@ -17,24 +17,24 @@ export default class PingTimer { ) { this.keepalive = keepalive * 1000 this.checkPing = checkPing - this.variant = variant + this.timer = getTimer(variant) this.reschedule() } clear() { - if (this.timer) { - getTimer(this.variant).clear(this.timer) - this.timer = null + if (this.timerId) { + this.timer.clear(this.timerId) + this.timerId = null } } reschedule() { this.clear() - this.timer = getTimer(this.variant).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) diff --git a/src/lib/get-timer.ts b/src/lib/get-timer.ts index 82d58cc3f..d1d927ed1 100644 --- a/src/lib/get-timer.ts +++ b/src/lib/get-timer.ts @@ -5,7 +5,7 @@ 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 -interface Timer { +export interface Timer { set: typeof setT clear: typeof clearT } From 811682802c68560d9f2b02b527a6b826d4cc3194 Mon Sep 17 00:00:00 2001 From: olso Date: Mon, 18 Mar 2024 10:05:05 +0100 Subject: [PATCH 3/3] fix: pingTimer test check correct timerId prop --- DEVELOPMENT.md | 5 +++++ test/pingTimer.ts | 9 ++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index e9cc8b839..6505d666d 100644 --- a/DEVELOPMENT.md +++ b/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: diff --git a/test/pingTimer.ts b/test/pingTimer.ts index 9cf0b2716..cd3369eea 100644 --- a/test/pingTimer.ts +++ b/test/pingTimer.ts @@ -18,7 +18,7 @@ describe('PingTimer', () => { const cb = spy() 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,7 +29,10 @@ 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', () => { @@ -52,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') }) })