From ffa2447d15bc99dbefd1bb815c4a2d2690f53b6c Mon Sep 17 00:00:00 2001 From: Evan Anderson Date: Fri, 22 Nov 2024 14:29:39 -0800 Subject: [PATCH] Fix flakes in ./topdown/cache Signed-off-by: Evan Anderson --- topdown/cache/cache.go | 6 ++---- topdown/cache/cache_test.go | 32 ++++++++++++++++++++++---------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/topdown/cache/cache.go b/topdown/cache/cache.go index 55ed340619..aa363cf539 100644 --- a/topdown/cache/cache.go +++ b/topdown/cache/cache.go @@ -154,16 +154,14 @@ func NewInterQueryCache(config *Config) InterQueryCache { func NewInterQueryCacheWithContext(ctx context.Context, config *Config) InterQueryCache { iqCache := newCache(config) if iqCache.staleEntryEvictionTimePeriodSeconds() > 0 { - cleanupTicker := time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second) go func() { + cleanupTicker := time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second) + defer cleanupTicker.Stop() for { select { case <-cleanupTicker.C: - cleanupTicker.Stop() iqCache.cleanStaleValues() - cleanupTicker = time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second) case <-ctx.Done(): - cleanupTicker.Stop() return } } diff --git a/topdown/cache/cache_test.go b/topdown/cache/cache_test.go index ede1983c44..1423a1b363 100644 --- a/topdown/cache/cache_test.go +++ b/topdown/cache/cache_test.go @@ -408,14 +408,17 @@ func TestInsertWithExpiryAndEviction(t *testing.T) { t.Fatalf("Unexpected error %v", err) } - cache := NewInterQueryCacheWithContext(context.Background(), config) + // This starts a background ticker at stale_entry_eviction_period_seconds to clean up items. + ctx, cancel := context.WithCancel(context.Background()) + cache := NewInterQueryCacheWithContext(ctx, config) + t.Cleanup(cancel) cacheValue := newInterQueryCacheValue(ast.StringTerm("bar").Value, 20) cache.InsertWithExpiry(ast.StringTerm("force_evicted_foo").Value, cacheValue, time.Now().Add(100*time.Second)) if fetchedCacheValue, found := cache.Get(ast.StringTerm("force_evicted_foo").Value); !found { t.Fatalf("Expected cache entry with value %v, found %v", cacheValue, fetchedCacheValue) } - cache.InsertWithExpiry(ast.StringTerm("expired_foo").Value, cacheValue, time.Now().Add(1*time.Second)) + cache.InsertWithExpiry(ast.StringTerm("expired_foo").Value, cacheValue, time.Now().Add(900*time.Millisecond)) if fetchedCacheValue, found := cache.Get(ast.StringTerm("expired_foo").Value); !found { t.Fatalf("Expected cache entry with value %v, found %v", cacheValue, fetchedCacheValue) } @@ -425,7 +428,7 @@ func TestInsertWithExpiryAndEviction(t *testing.T) { } // Ensure stale entries clean up routine runs at least once - time.Sleep(2 * time.Second) + time.Sleep(1100 * time.Millisecond) // Entry deleted even though not expired because force evicted when foo is inserted if fetchedCacheValue, found := cache.Get(ast.StringTerm("force_evicted_foo").Value); found { @@ -454,20 +457,23 @@ func TestInsertHighTTLWithStaleEntryCleanup(t *testing.T) { t.Fatalf("Unexpected error %v", err) } - cache := NewInterQueryCacheWithContext(context.Background(), config) + // This starts a background ticker at stale_entry_eviction_period_seconds to clean up items. + ctx, cancel := context.WithCancel(context.Background()) + cache := NewInterQueryCacheWithContext(ctx, config) + t.Cleanup(cancel) cacheValue := newInterQueryCacheValue(ast.StringTerm("bar").Value, 20) cache.InsertWithExpiry(ast.StringTerm("high_ttl_foo").Value, cacheValue, time.Now().Add(100*time.Second)) if fetchedCacheValue, found := cache.Get(ast.StringTerm("high_ttl_foo").Value); !found { t.Fatalf("Expected cache entry with value %v, found %v", cacheValue, fetchedCacheValue) } - cache.InsertWithExpiry(ast.StringTerm("expired_foo").Value, cacheValue, time.Now().Add(1*time.Second)) + cache.InsertWithExpiry(ast.StringTerm("expired_foo").Value, cacheValue, time.Now().Add(900*time.Millisecond)) if fetchedCacheValue, found := cache.Get(ast.StringTerm("expired_foo").Value); !found { t.Fatalf("Expected cache entry with value %v, found no entry", fetchedCacheValue) } // Ensure stale entries clean up routine runs at least once - time.Sleep(2 * time.Second) + time.Sleep(1100 * time.Millisecond) cache.InsertWithExpiry(ast.StringTerm("foo").Value, cacheValue, time.Now().Add(10*time.Second)) if fetchedCacheValue, found := cache.Get(ast.StringTerm("foo").Value); !found { @@ -497,7 +503,10 @@ func TestInsertHighTTLWithoutStaleEntryCleanup(t *testing.T) { t.Fatalf("Unexpected error %v", err) } - cache := NewInterQueryCacheWithContext(context.Background(), config) + // This starts a background ticker at stale_entry_eviction_period_seconds to clean up items. + ctx, cancel := context.WithCancel(context.Background()) + cache := NewInterQueryCacheWithContext(ctx, config) + t.Cleanup(cancel) cacheValue := newInterQueryCacheValue(ast.StringTerm("bar").Value, 20) cache.InsertWithExpiry(ast.StringTerm("high_ttl_foo").Value, cacheValue, time.Now().Add(100*time.Second)) @@ -537,14 +546,17 @@ func TestZeroExpiryTime(t *testing.T) { t.Fatalf("Unexpected error %v", err) } - cache := NewInterQueryCacheWithContext(context.Background(), config) + // This starts a background ticker at stale_entry_eviction_period_seconds to clean up items. + ctx, cancel := context.WithCancel(context.Background()) + cache := NewInterQueryCacheWithContext(ctx, config) + t.Cleanup(cancel) cacheValue := newInterQueryCacheValue(ast.StringTerm("bar").Value, 20) cache.InsertWithExpiry(ast.StringTerm("foo").Value, cacheValue, time.Time{}) if fetchedCacheValue, found := cache.Get(ast.StringTerm("foo").Value); !found { t.Fatalf("Expected cache entry with value %v for foo, found %v", cacheValue, fetchedCacheValue) } - time.Sleep(2 * time.Second) + time.Sleep(1100 * time.Millisecond) // Stale entry cleanup routine skips zero time cache entries if fetchedCacheValue, found := cache.Get(ast.StringTerm("foo").Value); !found { @@ -574,7 +586,7 @@ func TestCancelNewInterQueryCacheWithContext(t *testing.T) { } cancel() - time.Sleep(2 * time.Second) + time.Sleep(1100 * time.Millisecond) // Stale entry cleanup routine stopped as context was cancelled if fetchedCacheValue, found := cache.Get(ast.StringTerm("foo").Value); !found {