-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix several goroutine leaks #262
base: dev
Are you sure you want to change the base?
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 err I'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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good idea.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this close
is to end the range
loop inside the goroutine and let it exit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree this is needed to exit that goroutine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one line is the only part of this PR that has not already been done, or is perhaps not strictly needed if we update the docs as I mentioned in my comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TO clarify: I do think we still need this one line, and nothing else further.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This close
also exits the goroutine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call to close()
was added in #313
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 |
changes := sig.Body[1].(map[string]dbus.Variant) | ||
|
||
if connected, ok := changes["Connected"].Value().(bool); ok && !connected { | ||
c.EnableNotifications(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this result in a recursive call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a recursive call. Is there an issue with doing that?
If you're concerned about an infinite loop, the callback passed to c.EnableNotifications()
there is nil
, which means it'll run case nil
in the switch statement and not the default where this call is located, so there can't be a loop here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that, but it does make it harder for people to read.
Anyhow, this case could also be handled by having dev call EnableNotifications(nil)
themselves when closing without the extra logic being added. We can add a note to the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main problem I was trying to solve with that code is that devices can disconnect without the dev doing anything. For example, when they go out of range. That's a problem I was experiencing very frequently with my project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be handled by calling EnableNotifications(nil)
in the ConnectHandler
if the connected
argument is false
, but I personally feel that requiring a custom disconnect handler just to avoid a goroutine leak is extremely counterintuitive and I think people would be likely to miss that and end up with a really subtle goroutine leak that's very difficult to find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. Thanks for the clarification.
One way that could be handled might be to correctly hook up the connect handler for Linux Advertisers via some property handler ala #257 (not exactly that, but similar).
In the handler, you could call device.EnableNotifications(nil)
or whatever is the correct handling for your application when devices disconnect.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed using a connect handler in my second comment (you probably didn't see it because I wrote it after my clarification). I'll definitely implement it that way if you feel this code shouldn't be included in the project, and that would solve the problem, but I believe it would be extremely counterintuitive and likely to lead to subtle and difficult-to-find leaks in people's projects in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone has an application that does not use property notifications at all aka a beacon, they would do nothing.
If they have an advertiser that does allow connects, they probably want to disable notifications as devices connect/disconnect.
I do realize that Advertisers that do not allow connections are not yet implemented in this package, but I am planning to work on that soon.
Mainly I am hoping to keep the implementations consistent. If ensuring that notifications are disabled is automatic on Linux, but not elsewhere, that would be a bit confusing as well? I don't know the perfect answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, no problem. I'll implement it in a disconnect handler then. In that case, though, I think it should be made clear in the docs that if you use notifications and don't do that, it may result in a goroutine leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we still need to implement connect handler for Advertiser for that to work for you, unless your application is a Central?
In any case, some additional work is still required of some kind.
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.signal
channel is now closed, releasing the goroutine that waits for the device to connect.Fixes #260