tinygo-org / bluetooth

Cross-platform Bluetooth API for Go and TinyGo. Supports Linux, macOS, Windows, and bare metal using Nordic SoftDevice or HCI
https://tinygo.org
Other
744 stars 137 forks source link

Characteristic notification handler goroutine leak #260

Open Elara6331 opened 7 months ago

Elara6331 commented 7 months ago

When notifications are enabled on a DeviceCharacteristic on Linux, it starts a new goroutine listening on a channel for DBus signals, but when a device disconnects (or when EnableNotifications(nil) is called), the c.property channel is never closed, so the goroutine will keep waiting for a signal on its property channel.

In the case of EnableNotifications(nil), the goroutine will sleep forever and the signal will never come due to the call to RemoveSignal here:

https://github.com/tinygo-org/bluetooth/blob/12b6f0bc25b665a683fd3143cfaef76dadfb619b/gattc_linux.go#L282

However, when a device disconnects, RemoveSignal is never called, so when a new notification handler is added to that characteristic after the device reconnects, the MatchSignal is re-added, so the old goroutine starts receiving signals again and executes its old handler, causing duplicated events and other undesirable issues.

jasday commented 7 months ago

I saw similar behavior in v0.8.0, along with the memory leak in my issue here https://github.com/tinygo-org/bluetooth/issues/252.

I'm no longer seeing the memory leak as mentioned in my issue, but I am seeing a leak in v0.9.0 such as the one you mentioned here when connecting to a device and enabling notifications, along with undesirable events if EnableNotificaitons(nil) is not called

Elara6331 commented 7 months ago

Fixing the EnableNotifications(nil) leak is easy enough. Adding a close(c.property) before c.property = nil should fix it. The disconnect issue is more difficult to fix because you'd have to detect a device disconnect and release all the handler goroutines.