From 547519daa8353a2d8a7fe9e4ae715601570b085f Mon Sep 17 00:00:00 2001 From: olso-nordsec <99187211+olso-nordsec@users.noreply.github.com> Date: Mon, 18 Mar 2024 10:24:08 +0100 Subject: [PATCH] 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 --- DEVELOPMENT.md | 5 +++++ README.md | 1 + src/lib/PingTimer.ts | 24 ++++++++++++++++-------- src/lib/client.ts | 18 ++++++++++++++---- src/lib/get-timer.ts | 38 ++++++++++++++++++++++++++++++++++++++ src/lib/shared.ts | 2 ++ src/lib/timers.ts | 15 --------------- test/pingTimer.ts | 23 +++++++++++++++-------- 8 files changed, 91 insertions(+), 35 deletions(-) create mode 100644 src/lib/get-timer.ts delete mode 100644 src/lib/timers.ts 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/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..e5c7b9f09 100644 --- a/src/lib/PingTimer.ts +++ b/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) diff --git a/src/lib/client.ts b/src/lib/client.ts index d0da8419f..9bad1baf7 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 { @@ -2078,9 +2084,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..d1d927ed1 --- /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 + +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 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..cd3369eea 100644 --- a/test/pingTimer.ts +++ b/test/pingTimer.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') }) })