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

Conversation

@amrbekhit
Copy link

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 {}
}()

The use of ch to return the value of b.Parse here seems a bit clunky. It would be useful to just call WatchDeviceChanges and check for errors without needing to do any other maintenance except call UnwatchDeviceChanges when the beacon is removed. Is ch used to notify the caller that the beacon properties have changed so that they can take action? If so, is it critical that no events are missed such that a blocking send is required? If not, perhaps the blocking send to ch in the goroutine could be changed to a non-blocking one. This simplifies the caller code, as they can choose to ignore the channel if they don't need it:

if changed.Name == "ManufacturerData" || changed.Name == "ServiceData" {
    select {
        case ch <- b.Parse():
            // Result of parse sent to channel
        default:
            // The channel is blocked, so result is discarded
    }
}

The caller code is thus simplified:

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

// No need to empty the channel now

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 {}
	}()
```

The use of `ch` to return the value of `b.Parse` here seems a bit clunky. It would be useful to just call `WatchDeviceChanges` and check for errors without needing to do any other maintenance except call `UnwatchDeviceChanges` when the beacon is removed. Is `ch` used to notify the caller that the beacon properties have changed so that they can take action? If so, is it critical that no events are missed such that a blocking send is required? If not, perhaps the blocking send to `ch` in the goroutine could be changed to a non-blocking one. This simplifies the caller code, as they can choose to ignore the channel if they don't need it:

```go
    if changed.Name == "ManufacturerData" || changed.Name == "ServiceData" {
        select {
            case ch <- b.Parse():
                // Result of parse sent to channel
            default:
                // The channel is blocked, so result is discarded
        }
    }
```

The caller code is thus simplified:

```go
	_, err := b.WatchDeviceChanges(ctx)
	if err != nil {
        ...
    }

    // No need to empty the channel now
```
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