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

Make reporting of parse result non-blocking in WatchDeviceChanges #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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