chrvadala / node-ble

Bluetooth Low Energy (BLE) library written with pure Node.js (no bindings) - baked by Bluez via DBus
https://www.npmjs.com/package/node-ble
MIT License
310 stars 45 forks source link

Extend listener removal in BusHelper #37

Closed tuxedoxt closed 6 months ago

tuxedoxt commented 2 years ago

Hello,

if the intended use of Device.disconnect also is to clean up the remaining event listeners this PR extends this to include the leftover BusHelper._propsProxy event listeners adding a new function BusHelper.removeListeners for this purpose.

This seems to do it for my current issue in #35.

tuxedoxt commented 2 years ago

Bump :)

Any feedback?

chrvadala commented 2 years ago

This approach looks better, but I have to try it. I will next weekend, promised. In the mean time, can you add some unit test?

ChocolateLoverRaj commented 1 year ago

This seems to do it for my current issue in #35.

@tuxedoxt How did you get the error to stop? I tried using your fork and calling await device.disconnect(), but I still got error.

mxc42 commented 1 year ago

Related issue, after calling Adapter.devices in a loop once every 5 seconds I start to get (node:4278) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 {"path":"/org/bluez/hci0/dev_6C_EC_26_F4_E4_5C","interface":"org.freedesktop.DBus.Properties","member":"PropertiesChanged"} listeners added to [EventEmitter]. Use emitter.setMaxListeners() to increase limit

This is the code in question, called from a setInterval loop every 5 seconds.

    const devices = await adapter.devices();
    for (let i = 0; i < devices.length; i++) {
        try {

            const device = await adapter.getDevice(devices[i]);
            const name = await device.getName();
            const rssi = await device.getRSSI();
            await device.disconnect();
            ...

I've tried the changes in this PR but they didn't affect the error.

tuxedoxt commented 1 year ago

@tuxedoxt How did you get the error to stop? I tried using your fork and calling await device.disconnect(), but I still got error.

I ended up using this change combined with https://github.com/dbusjs/node-dbus-next/pull/110

Since none got merged still using the forked versions explicitly binding the github urls in package.json where needed.

Example, temporary forked node-ble which has this change and a reference to forked dbus-next change:

"node-ble": "git+https://github.com/tuxedoxt/node-ble.git#match-and-event-leak-fixes",
coveralls commented 1 year ago

Coverage Status

Coverage: 95.783% (-0.05%) from 95.833% when pulling 1855c3b4a59b785a6b1f89e69b0b54a11bca2ef6 on tuxedoxt:device-listener-cleanup into eb20e9c32d431df65ee5718c6c0e968b09524085 on chrvadala:main.

derwehr commented 1 year ago

Any chance this will get merged soon?

tuxedoxt commented 1 year ago

Another bump :)

@chrvadala

Leafgard commented 11 months ago

Is There Anybody Out There?

@chrvadala

chrvadala commented 6 months ago

Released with v1.10