EnableNotifications doesn't properly close underlying goroutines/channels

[sbinet]

on Linux, DeviceCharacteristic.EnableNotifications calls WatchProperties and StartNotify on gatt.GattCharacteristic1.

but (as #106 noted) it doesn't call StopNotify nor UnwatchProperties.
this means the goroutine spawned by WatchProperties isn't properly closed nor collected.

(and it seems to me, client code can't call EnableNotifications again, once the first set of notifications has been collected.
another Device has to be created and connected anew)

the current API doesn't seem to easily allow for early cancellation of notifications either.

bluez seems to inject a poison pill (a nil value) into the channel and close it when UnwatchProperties is called.
so we'd need to drain that channel inside EnableNotifications.

to achieve proper goroutine collection and support early cancellation, we could either:

• modify the signature of the callback function to return an error and specify a special value for early cancellation, or
• modify the signature of EnableNotifications to take an additional parameter (either a context.Context or chan struct{}) so user could notify us when to call StopNotify+UnwatchProperties

WDYT ?

(I don't know how to handle this for other platforms/OSes)

[aykevl]

Here is a third option: use the API as it exists today, but change the meaning of EnableNotifications slightly:

• When called with a non-nil callback, it will start notifications (if not already started) and set the given callback to be called when notifications are received.
• When called with a nil callback, notifications are stopped.

This has the benefit that it doesn't break the API, and in my opinion also better fits the underlying APIs (it certainly better fits the SoftDevice and Darwin APIs).

[sbinet]

ok. I can adapt #169 along these lines.

[sbinet]

alternatively, what about introducing (Stop|Disable)Notifications ?

the darwin code for EnableNotifications tests for a nil callback and errors out.
modifiying EnableNotifications as you proposed would break code in a more subtle way, wouldn't it ?

[aykevl]

modifiying EnableNotifications as you proposed would break code in a more subtle way, wouldn't it ?

In theory, yes. In practice, I doubt anybody is calling EnableNotifications with a nil callback precisely because it is currently not supported:

• It returns an error on MacOS.
• I'm not entirely sure what it'll do on Linux, but it'll probably crash with a nil pointer dereference.
• On the nrf SoftDevice, it looks like it will stop the callback from being called without actually stopping the notifications.
• It will lead to a nil pointer dereference on Windows (the callback is called without checking for a nil value).

So to me changing the behavior of EnableNotifications with a nil callback seems safe to me.