From 3f5f887317be4d6399c8e319056fbacffa34349a Mon Sep 17 00:00:00 2001
From: Brian Rak <brian.rak@broadcom.com>
Date: Thu, 21 Nov 2024 14:23:27 -0600
Subject: [PATCH] fix: Property Collector updates should be triggered on the
 empty FilterSet

The PropertyCollector update closure passed to WaitForUpdatesEx is only
triggered when a non-empty FilterSet occurs.
This is an issue when side effects need to be triggered inside the
update function such as notification mechanisms that an update was
acknowledged.

It should be the choice of the update closure to determine whether to
process the empty FilterSet or not.

Closes #3631

Signed-off-by: Brian Rak <brian.rak@broadcom.com>
---
 property/collector.go      | 13 +++++++--
 property/collector_test.go | 59 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/property/collector.go b/property/collector.go
index 9ee78d0d3..244796dfc 100644
--- a/property/collector.go
+++ b/property/collector.go
@@ -264,9 +264,12 @@ func (p *Collector) RetrieveOne(ctx context.Context, obj types.ManagedObjectRefe
 
 // WaitForUpdatesEx waits for any of the specified properties of the specified
 // managed object to change. It calls the specified function for every update it
-// receives. If this function returns false, it continues waiting for
+// receives an update - including the empty filter set, which can occur if no
+// objects are eligible for updates.
+//
+// If this function returns false, it continues waiting for
 // subsequent updates. If this function returns true, it stops waiting and
-// returns.
+// returns upon receiving the first non-empty filter set.
 //
 // If the Context is canceled, a call to CancelWaitForUpdates() is made and its
 // error value is returned.
@@ -313,6 +316,12 @@ func (p *Collector) WaitForUpdatesEx(
 			opts.Truncated = *set.Truncated
 		}
 
+		if len(set.FilterSet) == 0 {
+			// Trigger callbacks in case callers need to be notified
+			// of the empty filter set.
+			_ = onUpdatesFn(make([]types.ObjectUpdate, 0))
+		}
+
 		for _, fs := range set.FilterSet {
 			if opts.PropagateMissing {
 				for i := range fs.ObjectSet {
diff --git a/property/collector_test.go b/property/collector_test.go
index 06ca8c260..90b3ec7bd 100644
--- a/property/collector_test.go
+++ b/property/collector_test.go
@@ -18,6 +18,7 @@ package property_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -110,6 +111,64 @@ func TestWaitForUpdatesEx(t *testing.T) {
 	}, model)
 }
 
+func TestWaitForUpdatesExEmptyUpdateSet(t *testing.T) {
+	model := simulator.VPX()
+	model.Datacenter = 1
+	model.Cluster = 0
+	model.Pool = 0
+	model.Machine = 0
+	model.Autostart = false
+
+	simulator.Test(func(ctx context.Context, c *vim25.Client) {
+		// Set up the finder and get a VM.
+		finder := find.NewFinder(c, true)
+		datacenter, err := finder.DefaultDatacenter(ctx)
+		if err != nil {
+			t.Fatalf("default datacenter not found: %s", err.Error())
+		}
+		finder.SetDatacenter(datacenter)
+		vmList, err := finder.VirtualMachineList(ctx, "*")
+		if len(vmList) != 0 {
+			t.Fatalf("vmList != 0")
+		}
+
+		pc, err := property.DefaultCollector(c).Create(ctx)
+		if err != nil {
+			t.Fatalf("failed to create new property collector: %s", err.Error())
+		}
+
+		// Start a goroutine to wait for updates on any VMs.
+		// Since there are no VMs in the filter set, we expect to
+		// receive an empty update set.
+		chanResult := make(chan error)
+		cancelCtx, cancel := context.WithCancel(ctx)
+		go func() {
+			defer close(chanResult)
+			_ = pc.WaitForUpdatesEx(
+				cancelCtx,
+				&property.WaitOptions{},
+				func(updates []types.ObjectUpdate) bool {
+					var err error
+					if len(updates) > 0 {
+						err = fmt.Errorf("unexpected update")
+					}
+					chanResult <- err
+					cancel()
+					return true
+				})
+		}()
+
+		select {
+		case <-ctx.Done():
+			t.Fatalf("timed out while waiting for updates")
+		case err := <-chanResult:
+			if err != nil {
+				t.Fatalf("error while waiting for updates: %s", err.Error())
+			}
+		}
+	}, model)
+}
+
 func TestRetrievePropertiesOneAtATime(t *testing.T) {
 	model := simulator.VPX()
 	model.Datacenter = 1