diff --git a/utils/window/window.go b/utils/window/window.go index 86dba5b717df..f354b839e79c 100644 --- a/utils/window/window.go +++ b/utils/window/window.go @@ -33,7 +33,7 @@ type window[T any] struct { minSize int // mutex for synchronization - lock sync.RWMutex + lock sync.Mutex // elements in the window elements buffer.Deque[node[T]] } @@ -77,8 +77,9 @@ func (w *window[T]) Add(value T) { // Oldest returns the oldest element in the window. func (w *window[T]) Oldest() (T, bool) { - w.lock.RLock() - defer w.lock.RUnlock() + w.lock.Lock() + defer w.lock.Unlock() + w.removeStaleNodes() oldest, ok := w.elements.PeekLeft() if !ok { @@ -89,8 +90,9 @@ func (w *window[T]) Oldest() (T, bool) { // Length returns the number of elements in the window. func (w *window[T]) Length() int { - w.lock.RLock() - defer w.lock.RUnlock() + w.lock.Lock() + defer w.lock.Unlock() + w.removeStaleNodes() return w.elements.Len() } @@ -100,13 +102,9 @@ func (w *window[T]) removeStaleNodes() { // If we're beyond the expiry threshold, removeStaleNodes this node from our // window. Nodes are guaranteed to be strictly increasing in entry time, // so we can break this loop once we find the first non-stale one. - newest, ok := w.elements.PeekRight() - if !ok { - return - } for w.elements.Len() > w.minSize { oldest, ok := w.elements.PeekLeft() - if !ok || newest.entryTime.Sub(oldest.entryTime) <= w.ttl { + if !ok || w.clock.Time().Sub(oldest.entryTime) <= w.ttl { return } _, _ = w.elements.PopLeft() diff --git a/utils/window/window_test.go b/utils/window/window_test.go index 43bfad82997d..add4810670ef 100644 --- a/utils/window/window_test.go +++ b/utils/window/window_test.go @@ -127,7 +127,7 @@ func TestTTLAdd(t *testing.T) { require.Equal(4, oldest) } -// TestTTLLength tests that elements are not evicted on Length +// TestTTLLength tests that elements are evicted on Length func TestTTLLength(t *testing.T) { require := require.New(t) @@ -156,10 +156,10 @@ func TestTTLLength(t *testing.T) { clock.Set(start.Add(testTTL + time.Second)) // No more elements should be present in the window. - require.Equal(3, window.Length()) + require.Equal(0, window.Length()) } -// TestTTLOldest tests that stale elements are not evicted on calling Oldest +// TestTTLOldest tests that stale elements are evicted on calling Oldest func TestTTLOldest(t *testing.T) { require := require.New(t) @@ -188,15 +188,25 @@ func TestTTLOldest(t *testing.T) { require.Equal(1, oldest) require.Equal(3, window.elements.Len()) - // Now we're one second past the ttl of 10 seconds as defined in testTTL, + // Now we're one second before the ttl of 10 seconds as defined in testTTL, // so all existing elements shoud still exist. - clock.Set(start.Add(testTTL + time.Second)) + // Add 4 to the window to make it: + // [1, 2, 3, 4] + clock.Set(start.Add(testTTL - time.Second)) + window.Add(4) - // Now there should be three elements in the window oldest, ok = window.Oldest() require.True(ok) require.Equal(1, oldest) - require.Equal(3, window.elements.Len()) + require.Equal(4, window.elements.Len()) + + // Now we're one second past the ttl of the initial 3 elements + // call to oldest should now evict 1,2,3 and return 4. + clock.Set(start.Add(testTTL + time.Second)) + oldest, ok = window.Oldest() + require.True(ok) + require.Equal(4, oldest) + require.Equal(1, window.elements.Len()) } // Tests that we bound the amount of elements in the window