search

sandeepmistry / node-sensortag

Node.js lib for the TI SensorTag
MIT License
218 stars 111 forks source link

Memory Leak -- Listeners not released #83

Closed mitron6 closed 8 years ago

mitron6 commented 8 years ago

Repeated discoverAll/stopDiscoverAll with device advertising but not connecting fails to release a listener.

The following code duplicates the problem on both OSX and Raspian.

var sensortag = require('sensortag');

function onDiscover(sensor) {console.log('Found: ' + sensor.id)}

var scanning = false; var count = 0;

function discover() { if (scanning) { console.log('StopDiscoverAll\n'); sensortag.stopDiscoverAll(onDiscover) } scanning = true; sensortag.discoverAll(onDiscover); console.log(++count + '.\ndiscoverAll'); }

setInterval(discover, 2000);

Produces the following output: 1. discoverAll Found: 247189079301 StopDiscoverAll

2. discoverAll Found: 247189079301 StopDiscoverAll

3. discoverAll Found: 247189079301 StopDiscoverAll

4. discoverAll Found: 247189079301 StopDiscoverAll

5. discoverAll Found: 247189079301 StopDiscoverAll

6. discoverAll Found: 247189079301 StopDiscoverAll

7. discoverAll Found: 247189079301 StopDiscoverAll

8. discoverAll Found: 247189079301 StopDiscoverAll

9. discoverAll Found: 247189079301 StopDiscoverAll

10. discoverAll Found: 247189079301 StopDiscoverAll

11. discoverAll (node) warning: possible EventEmitter memory leak detected. 11 disconnect listeners added. Use emitter.setMaxListeners() to increase limit. Trace at Peripheral.addListener (events.js:239:17) at NobleDevice (/usr/src/app/node_modules/sensortag/node_modules/noble-device/lib/noble-device.js:18:20) at new CC2650SensorTag (/usr/src/app/node_modules/sensortag/lib/cc2650.js:30:15) at Noble.constructor.onDiscover (/usr/src/app/node_modules/sensortag/node_modules/noble-device/lib/util.js:28:22) at emitOne (events.js:82:20) at Noble.emit (events.js:169:7) at Noble.onDiscover (/usr/src/app/node_modules/sensortag/node_modules/noble-device/node_modules/noble/lib/noble.js:135:10) at emitMany (events.js:108:13) at emit (events.js:182:7) at NobleBindings.onDiscover (/usr/src/app/node_modules/sensortag/node_modules/noble-device/node_modules/noble/lib/hci-socket/bindings.js:169:10)

sandeepmistry commented 8 years ago

I've pushed a fix to noble-device: https://github.com/sandeepmistry/noble-device/commit/fb584807c5753ea57290f1303a629e45f3eb4ef1

Could you please try it out? Then I can publish a new version to npm.

mitron6 commented 8 years ago

It does not appear to work but the error message is slightly different.

(node) warning: possible EventEmitter memory leak detected. 11 disconnect listeners added. Use emitter.setMaxListeners() to increase limit. Trace at Peripheral.addListener (events.js:239:17) at Peripheral.once (events.js:265:8) at NobleDevice (/dev/sensortag/node_modules/noble-device/lib/noble-device.js:18:20) at new CC2650SensorTag (/dev/sensortag/lib/cc2650.js:30:15) at Noble.constructor.onDiscover (/dev/node_modules/sensortag/node_modules/noble-device/lib/util.js:28:22) at emitOne (events.js:82:20) at Noble.emit (events.js:169:7) at Noble.onDiscover (/dev/node_modules/sensortag/node_modules/noble-device/node_modules/noble/lib/noble.js:135:10) at emitMany (events.js:108:13) at emit (events.js:182:7)

danielbh commented 8 years ago

Confirming this doesn't work.

mitron6 commented 8 years ago

How about something like this?

   this.addressType = peripheral.addressType;
   this.connectedAndSetUp = false;

+  this._peripheral.removeAllListeners('disconnect');
   this._peripheral.on('disconnect', this.onDisconnect.bind(this));

Each emitter.once() adds a listener which is deleted after the first time the event is emitted. But if the devices are never connected they won't be disconnected and so the listener won't get deleted.

The above change fixes the problem in this issue. Does is create other problems?

sandeepmistry commented 8 years ago

See suggestion in https://github.com/sandeepmistry/noble-device/commit/fb584807c5753ea57290f1303a629e45f3eb4ef1#commitcomment-18752404

If that makes sense and works, please submit a PR to noble-device.

sandeepmistry commented 8 years ago

This has been fixed by https://github.com/sandeepmistry/noble-device/pull/22.

Which has been published to npm and included as a dependency for node-sensortag v1.2.3.