Race condition in Device.js / disconnect()

[nmasse-itix]

Environment

I'm using the following code to connect to a Lego Powered Up Hub using node-ble on a Framework Laptop 13 AMD running Fedora 40.

Code to reproduce the issue

const debug = require('debug')('sample-code');

const {createBluetooth} = require('node-ble');
const {bluetooth, destroy} = createBluetooth();

function sleep(delay) {
    return new Promise(function(resolve) {
        setTimeout(resolve, delay);
    });
}

async function main () {
    const adapter = await bluetooth.defaultAdapter();
    if (! await adapter.isDiscovering()) {
        await adapter.startDiscovery();
    }

    const device = await adapter.waitDevice('9C:9A:C0:02:E8:69');

    // Bind event listeners
    device.on("connect", (e) => { 
        debug("Connected !");
    });
    device.on("disconnect", (e) => { 
        debug("Disconnected !");
    });

    // Connect to the device
    await device.connect();
    
    // Do some work here
    sleep(2000);

    // Disconnect from the device
    await device.disconnect();

    // Do some heavy computation here
    for (i = 0; i < 1000000000; i++) {
        i = i * 1;
    }
 
    // Cleanup
    destroy();
    return "End of main";
}
main()
  .then(debug)
  .catch(debug)

Run it with :

while sleep 1; do
  DEBUG="sample-code" node index.js
  echo
done

Expected behavior

The following message is expected to appear on the console, until Ctrl+C is pressed.

  sample-code Connected ! +0ms
  sample-code Disconnected ! +3s
  sample-code End of main +2s

Current behavior

2 out of 5 disconnect events are lost.

$ while; sleep 1; do DEBUG="sample-code" node index.js; echo; done
  sample-code Connected ! +0ms
  sample-code Disconnected ! +3s
  sample-code End of main +2s

  sample-code Connected ! +0ms
  sample-code Disconnected ! +3s
  sample-code End of main +2s

  sample-code Connected ! +0ms
  sample-code End of main +5s

  sample-code Connected ! +0ms
  sample-code Disconnected ! +3s
  sample-code End of main +2s

  sample-code Connected ! +0ms
  sample-code End of main +5s

  sample-code Connected ! +0ms
^C

Possible explanation

In Device.js, in the disconnect method :

  /**
   * Disconnect remote device
   */
  async disconnect () {
    await this.helper.callMethod('Disconnect')
    this.helper.removeListeners()
  }

The removeListeners method is called right after the disconnect method is called. It leaves only a thin window where this.helper can send a disconnect event before the event listener are removed.

Depending on how your system is loaded / scheduled, the disconnect event might arrive after the event listeners have been removed.

Possible fix

Maybe remove all listeners after all disconnect events have been sent ?

  /**
   * Connect to remote device
   */
  async connect () {
    const cb = (propertiesChanged) => {
      if ('Connected' in propertiesChanged) {
        const { value } = propertiesChanged.Connected
        if (value) {
          this.emit('connect', { connected: true })
        } else {
          this.emit('disconnect', { connected: false })
+         this.helper.removeListeners()
        }
      }
    }

    this.helper.on('PropertiesChanged', cb)
    await this.helper.callMethod('Connect')
  }

  /**
   * Disconnect remote device
   */
  async disconnect () {
    await this.helper.callMethod('Disconnect')
-   this.helper.removeListeners()
  }

It does not match at 100% the previous API contract.

If the device has been disconnected externally (power off, weak signal, etc), currently the event handlers receives a disconnected event and a connect event as soon as the device comes back.

With the suggested fix, only a disconnected event is delivered.

What was the case for removing listeners upon explicit disconnection ?

[chrvadala]

Hi @nmasse-itix, unfortunately with the current design these listeners have to be removed, otherwise there's a potential memory leak. They were introduced by this PR https://github.com/chrvadala/node-ble/issues/72. I think that the current solution might work for most cases, but I'm looking for something better. Can you create a unit test that reproduce this issue? Moreover, I'm open to ideas that could fix.