muka / go-bluetooth

Golang bluetooth client based on bluez DBus interfaces
Apache License 2.0
653 stars 123 forks source link

Fix beacon WatchDeviceChanges memory leak #183

Open amrbekhit opened 1 year ago

amrbekhit commented 1 year ago

The WatchDeviceChanges function creates a goroutine that watches for any change notifications coming from the underlying Device. The result of b.Parse is sent to the channel ch using a blocking send. Because of this, the caller to WatchDeviceChanges must continuously empty this channel otherise the goroutine will simply hang on this line, meaning it will never exit and result in a goroutine leak. So the caller code looks something like this:

ch, err := b.WatchDeviceChanges(ctx)
if err != nil {
    ...
}

// Make sure to empty the channel
go func() {
    for range ch {}
}()

This brings us onto the second issue: the channel ch is only closed when the context ctx is done. However, the goroutine can also exit due to b.propchanged sending a nil, which occurs when UnwatchDeviceChanges is called. In this case, ch is never closed, which means the caller to WatchDeviceChanges that is emptying ch will never quit, again resulting in a goroutine and channel leak. Closing the channel using a defer at the beginning of the goroutine resolves this issue.

In addition, when the context reports that it is done, instead of returning from the function, it just breaks, which continues the loop and can potentially cause a panic if it tries to write to the channel ch.

The call to ctx.Done is meaningless on line 66. I'm guessing the intention was to cancel the context, which is not possible from this function.