Fix several goroutine leaks #262
Conversation
|
I've done some testing on this branch and seem to be getting some interesting results:
This seems to only be a problem when the device is connected to repeatedly without another connection in between. If I connect to one device, receive notifications, disconnect, connect to different device, read notifications, disconnect, I am able to receive notifications from the original device again. Sometimes repeating to the same device repeatedly will result in a notification being read, but it seems to be slightly random when it will work.
This was tested on multiple devices, and occurred on both although not consistently. That being said, I get this on the release version, just not as often - so potentially a different problem that this fix is revealing? |
Hmm, that's weird. I've specifically tested that case several times and it seems to work for me. I also see no reason that this PR would break that, since the DBus match rules and signal channel are set up again when you re-enable notifications. Here's a simplified reproduction that I used to test this: main.gopackage main
import (
"log"
"time"
"tinygo.org/x/bluetooth"
)
const address = "ED:47:AC:47:F4:FB"
var (
serviceUUID = mustParse("00000000-78fc-48fe-8e23-433b3a1942d0")
eventCharUUID = mustParse("00000001-78fc-48fe-8e23-433b3a1942d0")
)
func main() {
adapter := bluetooth.DefaultAdapter
err := adapter.Enable()
if err != nil {
panic(err)
}
device := connectDevice(adapter)
char := getChar(device)
log.Println("First connection complete")
err = char.EnableNotifications(func(buf []byte) {
log.Printf("Received notification: %x\n", buf)
})
if err != nil {
panic(err)
}
log.Println("Notifications enabled")
time.Sleep(2*time.Second)
err = device.Disconnect()
if err != nil {
panic(err)
}
log.Println("Device disconnected")
time.Sleep(2*time.Second)
device = connectDevice(adapter)
char = getChar(device)
log.Println("Second connection complete")
err = char.EnableNotifications(func(buf []byte) {
log.Printf("Received notification: %x\n", buf)
})
if err != nil {
panic(err)
}
log.Println("Notifications re-enabled; sleeping forever")
select {}
}
func connectDevice(adapter *bluetooth.Adapter) bluetooth.Device {
var (
err error
device bluetooth.Device
)
err = adapter.Scan(func(adapter *bluetooth.Adapter, sr bluetooth.ScanResult) {
if sr.Address.String() == address {
device, err = adapter.Connect(sr.Address, bluetooth.ConnectionParams{})
if err != nil {
panic(err)
}
adapter.StopScan()
}
})
if err != nil {
panic(err)
}
return device
}
func getChar(device bluetooth.Device) bluetooth.DeviceCharacteristic {
services, err := device.DiscoverServices([]bluetooth.UUID{serviceUUID})
if err != nil {
panic(err)
}
chars, err := services[0].DiscoverCharacteristics([]bluetooth.UUID{eventCharUUID})
if err != nil {
panic(err)
}
return chars[0]
}
func mustParse(s string) bluetooth.UUID {
uuid, err := bluetooth.ParseUUID(s)
if err != nil {
panic(err)
}
return uuid
}
I've also had this happen intermittently with muka/go-bluetooth and |
|
I did find a different problem though: Line 239 in 4557b5d The problem here is that the receiver is not a pointer, so the Lines 250 to 253 in 4557b5d those changes don't persist, which means running There are two ways to fix this. The cleaner way would be to change the I'm not sure which approach this library would rather go with, so I won't change this until a maintainer of the project weighs in. |
That's interesting, I tried with your code and got this output log: I was taking readings (using a health thermometer device) and expected notifications to appear after the second notification re-enabled, but received nothing. I'm assuming I should be seeing notifications appear here as long as the device is still active and taking measurements? Doing a little more testing, I have two devices with the same service/characteristic and one different. On the first connection of one of the matching pair, I receive a single notification with value, and then no more. I can then connect the other of the pair, and get the same behavior. If I then connect a different type of device, with a different service/characteristic, I can get only the first values from that - however connecting this new service seems to do some type of 'reset' and I can then receive the first value from both the original devices again. The only real difference I'm seeing between your example code and mine is that I don't stop scanning - I need to be able to listen for devices whilst I'm handling a scan result, but adding a stopScan to mine still produces the same results. |
That's weird. It's been working consistently for me. I've tried it on Intel, MediaTek, and Realtek bluetooth adapters, and with different peripherals. Here's my log: |
|
I'm testing on an NXP IW416 chip - not sure if this would cause any differences though. Firmware is up to date, and the code looks like it should be working. Could this specific chip be sending extra signals that is causing notifications to be disabled? I've attached my bluez logs in case it's a possibility. The device I'm testing with is |
|
I've done a bit of testing on a local branch, and seem to have got it to work on my device. It only needs a small change to the nil case in the same switch statement, adding err := c.characteristic.Call("org.bluez.GattCharacteristic1.StopNotify", 0).Err
if err != nil {
return err
}to make the entire case case nil:
if c.property == nil {
return nil
}
err := c.characteristic.Call("org.bluez.GattCharacteristic1.StopNotify", 0).Err
if err != nil {
return err
}
err = c.adapter.bus.RemoveMatchSignal(c.propertiesChangedMatchOption)
c.adapter.bus.RemoveSignal(c.property)
close(c.property)
c.property = nil
return errI'm not entirely sure the side affect of this, if we error when calling stop notify this may cause the signals to not be removed properly (or, on the other hand, will this mean that it will just try again if it fails?) - @Elara6331 do you have any thoughts? If this change could be tested/confirmed and then added to the PR if proven working, that would be great. |
| @@ -345,6 +345,7 @@ func (a *Adapter) Connect(address Address, params ConnectionParams) (Device, err | |||
| // were connected between the two calls the signal wouldn't be picked up. | |||
| signal := make(chan *dbus.Signal) | |||
| a.bus.Signal(signal) | |||
| defer close(signal) | |||
There was a problem hiding this comment.
Is this close needed?
Note that the only thing closing a channel does is signal to receivers that the channel is closed (and makes sending on a channel panic). It is not needed to recover resources: a channel will be garbage collected like any other object.
There was a problem hiding this comment.
The reason for this close is to end the range loop inside the goroutine and let it exit
| @@ -280,6 +293,7 @@ func (c DeviceCharacteristic) EnableNotifications(callback func(buf []byte)) err | |||
|
|
|||
| err := c.adapter.bus.RemoveMatchSignal(c.propertiesChangedMatchOption) | |||
| c.adapter.bus.RemoveSignal(c.property) | |||
| close(c.property) | |||
There was a problem hiding this comment.
This close also exits the goroutine
|
I do have a question about this: Line 239 in 4f54e74 Since the method doesn't use a pointer receiver, the following lines operate on a copy of the characteristic and make Line 250 in 4f54e74 Line 252 in 4f54e74 I'm not sure what the best way to solve this would be, since making it a pointer receiver would be a breaking change and inconsistent with the other implementations. |
|
Pinging @aykevl for an opinion on this, please. |
|
Have there been any more thoughts on this? Along with the stop notifiy addition? @aykevl @Elara6331 |
This PR fixes several goroutine leaks that I've found:
EnableNotifications(nil)is called on aDeviceCharacteristic, the property channel is now closed, which releases the notification handler goroutine and allows it to exit.signalchannel is now closed, releasing the goroutine that waits for the device to connect.Fixes #260