Fix a race condition in startNotification with the "valuechanged" event notifications (#77)

* see #72: fix a race condition in startNotification with the "valuechanged" event notifications

* fix styling issues

Author: nmasse-itix

README.md

@@ -100,16 +100,16 @@ console.log(buffer)
 ```javascript
 const service2 = await gattServer.getPrimaryService('uuid')
 const characteristic2 = await service2.getCharacteristic('uuid')
-await characteristic2.startNotifications()
 characteristic2.on('valuechanged', buffer => {
   console.log(buffer)
 })
-await characteristic2.stopNotifications()
+await characteristic2.startNotifications()
 ```
 
 ## STEP 5: Disconnect
-When you have done you can disconnect and destroy the session.
+When you have done you can stop notifications, disconnect and destroy the session.
 ```javascript
+await characteristic2.stopNotifications()
 await device.disconnect()
 destroy()
 ```

src/GattCharacteristic.js

@@ -105,8 +105,6 @@ class GattCharacteristic extends EventEmitter {
    * It emits valuechanged event when receives a notification.
    */
   async startNotifications () {
-    await this.helper.callMethod('StartNotify')
-
     const cb = (propertiesChanged) => {
       if ('Value' in propertiesChanged) {
         const { value } = propertiesChanged.Value
@@ -115,6 +113,8 @@ class GattCharacteristic extends EventEmitter {
     }
 
     this.helper.on('PropertiesChanged', cb)
+
+    await this.helper.callMethod('StartNotify')
   }
 
   async stopNotifications () {

test/GattCharacteristic.spec.js

@@ -100,3 +100,36 @@ test('event:valuechanged', async () => {
 
   await characteristic.stopNotifications()
 })
+
+test('race condition between event:valuechanged / startNotification', async () => {
+  const characteristic = new GattCharacteristic(dbus, 'hci0', 'dev_00_00_00_00_00_00', 'characteristic0006', 'char008')
+  const cb = jest.fn(value => {})
+  characteristic.on('valuechanged', cb)
+
+  // Wrap the call to StartNotify with an early property change to check this event is not lost in a race condition
+  characteristic.helper.callMethod.mockImplementationOnce(async (method) => {
+    if (method !== 'StartNotify') {
+      throw new Error('Unexpected state in unit test')
+    }
+
+    await characteristic.helper.callMethod('StartNotify')
+
+    // Send the first event right after StartNotify
+    characteristic.helper.emit('PropertiesChanged',
+      { Value: { signature: 'ay', value: [0x62, 0x61, 0x72] } } // means bar
+    )
+  })
+
+  // Start notifications, wait 200ms and send a second event
+  characteristic.startNotifications()
+  await new Promise((resolve) => setTimeout(resolve, 200))
+  characteristic.helper.emit('PropertiesChanged',
+    { Value: { signature: 'ay', value: [0x62, 0x61, 0x72] } } // means bar
+  )
+
+  // Check the mocked callback function has been called twice
+  expect(cb.mock.calls).toHaveLength(2)
+
+  // Cleanup
+  characteristic.off('valuechanged', cb)
+})