Skip to content

Commit 0f3b351

Browse files
PiAreSquaredPiAreSquared
authored
fix: correctly notify auto-cleaner when an item's TTL is shortened (#206)
1 parent 87a96cb commit 0f3b351

2 files changed

Lines changed: 116 additions & 39 deletions

File tree

‎cache.go‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,11 @@ func New[K comparable, V any](opts ...Option[K, V]) *Cache[K, V] {
8888

8989
// updateExpirations updates the expiration queue and notifies
9090
// the cache auto cleaner if needed.
91+
// 'oldExpiresAt' should reflect the front of the expiration queue
92+
// before any item mutations.
9193
// Not safe for concurrent use by multiple goroutines without additional
9294
// locking.
93-
func (c *Cache[K, V]) updateExpirations(fresh bool, elem *list.Element) {
94-
var oldExpiresAt time.Time
95-
96-
if !c.items.expQueue.isEmpty() {
97-
oldExpiresAt = c.items.expQueue[0].Value.(*Item[K, V]).expiresAt
98-
}
95+
func (c *Cache[K, V]) updateExpirations(fresh bool, elem *list.Element, oldExpiresAt time.Time) {
9996

10097
if fresh {
10198
c.items.expQueue.push(elem)
@@ -151,9 +148,14 @@ func (c *Cache[K, V]) set(key K, value V, ttl time.Duration) *Item[K, V] {
151148
item := elem.Value.(*Item[K, V])
152149
oldItemCost := item.cost
153150

151+
var oldExpiresAt time.Time
152+
if !c.items.expQueue.isEmpty() {
153+
oldExpiresAt = c.items.expQueue[0].Value.(*Item[K, V]).expiresAt
154+
}
155+
154156
item.update(value, ttl)
155157

156-
c.updateExpirations(false, elem)
158+
c.updateExpirations(false, elem, oldExpiresAt)
157159

158160
if c.options.maxCost != 0 {
159161
c.cost = c.cost - oldItemCost + item.cost
@@ -185,11 +187,16 @@ func (c *Cache[K, V]) set(key K, value V, ttl time.Duration) *Item[K, V] {
185187
ttl = c.options.ttl
186188
}
187189

190+
var oldExpiresAt time.Time
191+
if !c.items.expQueue.isEmpty() {
192+
oldExpiresAt = c.items.expQueue[0].Value.(*Item[K, V]).expiresAt
193+
}
194+
188195
// create a new item
189196
item := NewItemWithOpts(key, value, ttl, c.options.itemOpts...)
190197
elem = c.items.lru.PushFront(item)
191198
c.items.values[key] = elem
192-
c.updateExpirations(true, elem)
199+
c.updateExpirations(true, elem, oldExpiresAt)
193200

194201
if c.options.maxCost != 0 {
195202
c.cost += item.cost
@@ -231,8 +238,13 @@ func (c *Cache[K, V]) get(key K, touch bool, includeExpired bool) *list.Element
231238
c.items.lru.MoveToFront(elem)
232239

233240
if touch && item.ttl > 0 {
241+
var oldExpiresAt time.Time
242+
if !c.items.expQueue.isEmpty() {
243+
oldExpiresAt = c.items.expQueue[0].Value.(*Item[K, V]).expiresAt
244+
}
245+
234246
item.touch()
235-
c.updateExpirations(false, elem)
247+
c.updateExpirations(false, elem, oldExpiresAt)
236248
}
237249

238250
return elem

‎cache_test.go‎

Lines changed: 95 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ func Test_Cache_updateExpirations(t *testing.T) {
4444
TimerChValue time.Duration
4545
Fresh bool
4646
EmptyQueue bool
47+
SingleItem bool
4748
OldExpiresAt time.Time
4849
NewExpiresAt time.Time
4950
Result time.Duration
@@ -116,6 +117,12 @@ func Test_Cache_updateExpirations(t *testing.T) {
116117
NewExpiresAt: newExp,
117118
Result: time.Until(newExp),
118119
},
120+
"Update with non fresh item, single item in queue and shortened expiresAt field": {
121+
SingleItem: true,
122+
OldExpiresAt: oldExp,
123+
NewExpiresAt: newExp,
124+
Result: time.Until(newExp),
125+
},
119126
}
120127

121128
for cn, c := range cc {
@@ -137,11 +144,13 @@ func Test_Cache_updateExpirations(t *testing.T) {
137144
}
138145

139146
if !c.EmptyQueue {
140-
cache.items.expQueue.push(&list.Element{
141-
Value: &Item[string, string]{
142-
expiresAt: c.OldExpiresAt,
143-
},
144-
})
147+
if !c.SingleItem {
148+
cache.items.expQueue.push(&list.Element{
149+
Value: &Item[string, string]{
150+
expiresAt: c.OldExpiresAt,
151+
},
152+
})
153+
}
145154

146155
if !c.Fresh {
147156
elem = &list.Element{
@@ -155,7 +164,11 @@ func Test_Cache_updateExpirations(t *testing.T) {
155164
}
156165
}
157166

158-
cache.updateExpirations(c.Fresh, elem)
167+
var oldExpiresAt time.Time
168+
if !c.EmptyQueue {
169+
oldExpiresAt = c.OldExpiresAt
170+
}
171+
cache.updateExpirations(c.Fresh, elem, oldExpiresAt)
159172

160173
var res time.Duration
161174

@@ -173,13 +186,14 @@ func Test_Cache_set(t *testing.T) {
173186
const newKey, existingKey, evictedKey = "newKey123", "existingKey", "evicted"
174187

175188
cc := map[string]struct {
176-
Capacity uint64
177-
MaxCost uint64
178-
Key string
179-
TTL time.Duration
180-
Metrics Metrics
181-
InsertCalled bool
182-
UpdateCalled bool
189+
Capacity uint64
190+
MaxCost uint64
191+
Key string
192+
TTL time.Duration
193+
Metrics Metrics
194+
InsertCalled bool
195+
UpdateCalled bool
196+
ExpectedTimerNotification time.Duration
183197
}{
184198
"Set with existing key and custom TTL": {
185199
Key: existingKey,
@@ -295,6 +309,24 @@ func Test_Cache_set(t *testing.T) {
295309
Evictions: 1,
296310
},
297311
},
312+
"Set with existing key and shortened TTL": {
313+
Key: existingKey,
314+
TTL: time.Minute,
315+
Metrics: Metrics{
316+
Updates: 1,
317+
},
318+
UpdateCalled: true,
319+
ExpectedTimerNotification: time.Minute,
320+
},
321+
"Set with new key and shortened TTL": {
322+
Key: newKey,
323+
TTL: time.Minute,
324+
Metrics: Metrics{
325+
Insertions: 1,
326+
},
327+
InsertCalled: true,
328+
ExpectedTimerNotification: time.Minute,
329+
},
298330
}
299331

300332
for cn, c := range cc {
@@ -385,6 +417,16 @@ func Test_Cache_set(t *testing.T) {
385417
assert.Zero(t, item.expiresAt)
386418
assert.NotEqual(t, c.Key, cache.items.expQueue[0].Value.(*Item[string, string]).key)
387419
}
420+
421+
if c.ExpectedTimerNotification > 0 {
422+
var res time.Duration
423+
select {
424+
case res = <-cache.items.timerCh:
425+
default:
426+
t.Fatal("expected timer notification but channel was empty")
427+
}
428+
assert.InDelta(t, c.ExpectedTimerNotification, res, float64(time.Second))
429+
}
388430
})
389431
}
390432

@@ -423,28 +465,37 @@ func Test_Cache_get(t *testing.T) {
423465
const existingKey, notFoundKey, expiredKey = "existing", "notfound", "expired"
424466

425467
cc := map[string]struct {
426-
Key string
427-
Touch bool
428-
WithTTL bool
468+
Key string
469+
Touch bool
470+
TTL time.Duration
471+
AddExpiredKey bool
472+
ExpectedTimerNotification time.Duration
429473
}{
430474
"Retrieval of non-existent item": {
431475
Key: notFoundKey,
432476
},
433477
"Retrieval of expired item": {
434-
Key: expiredKey,
478+
Key: expiredKey,
479+
AddExpiredKey: true,
435480
},
436481
"Retrieval of existing item without update": {
437482
Key: existingKey,
438483
},
439484
"Retrieval of existing item with touch and non zero TTL": {
440-
Key: existingKey,
441-
Touch: true,
442-
WithTTL: true,
485+
Key: existingKey,
486+
Touch: true,
487+
TTL: time.Hour * 30,
443488
},
444489
"Retrieval of existing item with touch and zero TTL": {
445490
Key: existingKey,
446491
Touch: true,
447492
},
493+
"Retrieval of existing item with touch and shortened TTL": {
494+
Key: existingKey,
495+
Touch: true,
496+
TTL: time.Millisecond,
497+
ExpectedTimerNotification: time.Millisecond,
498+
},
448499
}
449500

450501
for cn, c := range cc {
@@ -454,18 +505,16 @@ func Test_Cache_get(t *testing.T) {
454505
t.Parallel()
455506

456507
cache := prepCache(0, time.Hour, existingKey, "test2", "test3")
457-
addExpiredCacheItems(cache, expiredKey)
458-
time.Sleep(time.Millisecond) // force expiration
508+
if c.AddExpiredKey {
509+
addExpiredCacheItems(cache, expiredKey)
510+
time.Sleep(time.Millisecond) // force expiration
511+
}
459512

460513
oldItem := cache.items.values[existingKey].Value.(*Item[string, string])
461514
oldQueueIndex := oldItem.queueIndex
462515
oldExpiresAt := oldItem.expiresAt
463516

464-
if c.WithTTL {
465-
oldItem.ttl = time.Hour * 30
466-
} else {
467-
oldItem.ttl = 0
468-
}
517+
oldItem.ttl = c.TTL
469518

470519
elem := cache.get(c.Key, c.Touch, false)
471520

@@ -483,14 +532,30 @@ func Test_Cache_get(t *testing.T) {
483532
require.NotNil(t, elem)
484533
item := elem.Value.(*Item[string, string])
485534

486-
if c.Touch && c.WithTTL {
487-
assert.True(t, item.expiresAt.After(oldExpiresAt))
488-
assert.NotEqual(t, oldQueueIndex, item.queueIndex)
535+
if c.Touch && c.TTL > 0 {
536+
if item.expiresAt.Before(oldExpiresAt) {
537+
assert.Equal(t, oldQueueIndex, item.queueIndex)
538+
} else {
539+
assert.True(t, item.expiresAt.After(oldExpiresAt))
540+
assert.NotEqual(t, oldQueueIndex, item.queueIndex)
541+
}
489542
} else {
490543
assert.True(t, item.expiresAt.Equal(oldExpiresAt))
491544
assert.Equal(t, oldQueueIndex, item.queueIndex)
492545
}
493546

547+
select {
548+
case res := <-cache.items.timerCh:
549+
if c.ExpectedTimerNotification == 0 {
550+
t.Fatalf("unexpected timer notification: %v", res)
551+
}
552+
assert.InDelta(t, c.ExpectedTimerNotification, res, float64(time.Second))
553+
default:
554+
if c.ExpectedTimerNotification > 0 {
555+
t.Fatal("expected timer notification but channel was empty")
556+
}
557+
}
558+
494559
assert.Equal(t, c.Key, cache.items.lru.Front().Value.(*Item[string, string]).key)
495560
})
496561
}

0 commit comments

Comments
 (0)