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

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