Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Fix beacon WatchDeviceChanges memory leak #183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amrbekhit
Copy link

@amrbekhit amrbekhit commented Mar 24, 2023

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.

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:

```go
	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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant