From 4b360fe8e5ef402f50b01f51f8a13d621031fcc7 Mon Sep 17 00:00:00 2001 From: Christoffer Sandberg Date: Fri, 27 May 2022 11:33:32 +0200 Subject: [PATCH 1/4] Extend listener removal in BusHelper - Adds a clean-up function `removeListeners` that in addition to the already existing PropertiesChanged removal on itself (in `Device.disconnect`) also removes PropertiesChanged event listeners on the _propsProxy object. - Adds use of this clean-up function in `Device.disconnect`. --- src/BusHelper.js | 7 +++++++ src/Device.js | 2 +- test/Device.spec.js | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/BusHelper.js b/src/BusHelper.js index ae87ea6..9a6d78e 100644 --- a/src/BusHelper.js +++ b/src/BusHelper.js @@ -97,6 +97,13 @@ class BusHelper extends EventEmitter { return this._ifaceProxy[methodName](...args) } + removeListeners () { + this.removeAllListeners('PropertiesChanged') + if (this._propsProxy !== null) { + this._propsProxy.removeAllListeners('PropertiesChanged') + } + } + static buildChildren (path, nodes) { if (path === '/') path = '' const children = new Set() diff --git a/src/Device.js b/src/Device.js index 9142620..23154c6 100644 --- a/src/Device.js +++ b/src/Device.js @@ -119,7 +119,7 @@ class Device extends EventEmitter { */ async disconnect () { await this.helper.callMethod('Disconnect') - this.helper.removeAllListeners('PropertiesChanged') // might be improved + this.helper.removeListeners() } /** diff --git a/test/Device.spec.js b/test/Device.spec.js index 0cd342e..0679e03 100644 --- a/test/Device.spec.js +++ b/test/Device.spec.js @@ -13,6 +13,7 @@ jest.doMock('../src/BusHelper', () => { this.waitPropChange = jest.fn() this.children = jest.fn() this.callMethod = jest.fn() + this.removeListeners = jest.fn() } } }) From 35eb2e7f7a6d2929dc73c19c69633f5654f488e4 Mon Sep 17 00:00:00 2001 From: Christoffer Sandberg Date: Tue, 8 Nov 2022 17:25:28 +0100 Subject: [PATCH 2/4] Fix re-use after listener removal in BusHelper --- src/BusHelper.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/BusHelper.js b/src/BusHelper.js index 9a6d78e..f29da07 100644 --- a/src/BusHelper.js +++ b/src/BusHelper.js @@ -101,6 +101,7 @@ class BusHelper extends EventEmitter { this.removeAllListeners('PropertiesChanged') if (this._propsProxy !== null) { this._propsProxy.removeAllListeners('PropertiesChanged') + this._ready = false } } From 9ec2b04bac5b17f29b70b3211b710e2cb0eadac7 Mon Sep 17 00:00:00 2001 From: Christoffer Sandberg Date: Tue, 8 Nov 2022 17:33:28 +0100 Subject: [PATCH 3/4] Add test cases for BusHelper.removeListeners --- test/BusHelper.spec.js | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/BusHelper.spec.js b/test/BusHelper.spec.js index 8e594f3..fbf0da9 100644 --- a/test/BusHelper.spec.js +++ b/test/BusHelper.spec.js @@ -125,3 +125,31 @@ test('propsEvents', async () => { await helper.set('VirtualProperty', buildTypedValue('string', 'bar')) await expect(res).resolves.toMatchObject({ VirtualProperty: { signature: 's', value: 'bar' } }) }) + +test('removeListeners', async () => { + const helper = new BusHelper(dbus, TEST_NAME, TEST_OBJECT, TEST_IFACE, { useProps: true, usePropsEvents: true }) + + const dummyCb = () => {} + + // Init with listener on helper (directly attached dummyCb) and _propsProxy (through method call triggered _prepare) + helper.on('PropertiesChanged', dummyCb) + await helper.callMethod('Echo', 'ping') + expect(helper.listenerCount('PropertiesChanged')).toBeGreaterThan(0) + expect(helper._propsProxy.listenerCount('PropertiesChanged')).toBeGreaterThan(0) + + // Test remove + helper.removeListeners() + expect(helper.listenerCount('PropertiesChanged')).toBe(0) + expect(helper._propsProxy.listenerCount('PropertiesChanged')).toBe(0) + + // Test reuse after remove (same initialization as before) + helper.on('PropertiesChanged', dummyCb) + await helper.callMethod('Echo', 'ping') + expect(helper.listenerCount('PropertiesChanged')).toBeGreaterThan(0) + expect(helper._propsProxy.listenerCount('PropertiesChanged')).toBeGreaterThan(0) + + // Remove second time + helper.removeListeners() + expect(helper.listenerCount('PropertiesChanged')).toBe(0) + expect(helper._propsProxy.listenerCount('PropertiesChanged')).toBe(0) +}) From 89b0bb8ee3bf1e2fa078121f77c2dfa77ba0cdeb Mon Sep 17 00:00:00 2001 From: Christoffer Sandberg Date: Fri, 12 Jan 2024 17:12:16 +0100 Subject: [PATCH 4/4] Device: Change cleanup approach Disconnect cannot be used for cleanup as it actually has the property of disconnecting already connected devices. This commit removes listener removal from disconnect and places it in its own function that should be called on a Device object after instantiating it and before discarding it. --- src/Adapter.js | 2 ++ src/Device.js | 9 ++++++++- src/index.d.ts | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Adapter.js b/src/Adapter.js index f18f26c..be80427 100644 --- a/src/Adapter.js +++ b/src/Adapter.js @@ -109,6 +109,8 @@ class Adapter { /** * Init a device instance and returns it. + * Caller is responsible of calling cleanup() on the + * returned Device before discarding it to free resources. * @param {string} uuid - Device Name. * @async * @returns {Device} diff --git a/src/Device.js b/src/Device.js index 23154c6..8c9630e 100644 --- a/src/Device.js +++ b/src/Device.js @@ -119,7 +119,7 @@ class Device extends EventEmitter { */ async disconnect () { await this.helper.callMethod('Disconnect') - this.helper.removeListeners() + await this.cleanup() } /** @@ -142,6 +142,13 @@ class Device extends EventEmitter { return `${name} [${address}]` } + + /** + * Cleanup when discarding device object + */ + async cleanup () { + this.helper.removeListeners() + } } /** diff --git a/src/index.d.ts b/src/index.d.ts index b9a3507..73b5b9d 100644 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -45,6 +45,7 @@ declare namespace NodeBle { disconnect(): Promise; gatt(): Promise; toString(): Promise; + cleanup(): Promise; on(event: 'connect', listener: (state: ConnectionState) => void): this; on(event: 'disconnect', listener: (state: ConnectionState) => void): this;