Skip to content

Commit

Permalink
fix nasty bug
Browse files Browse the repository at this point in the history
This needs to be the opposite check of what we do in Get(). Introduced
in a fixup commit.
  • Loading branch information
mikedanese committed Nov 16, 2019
1 parent d16dde3 commit cc76b1a
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 2 deletions.
4 changes: 2 additions & 2 deletions staging/src/k8s.io/apimachinery/pkg/util/cache/expiring.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (c *Expiring) Get(key interface{}) (val interface{}, ok bool) {
c.mu.RLock()
defer c.mu.RUnlock()
e, ok := c.cache[key]
if !ok || c.clock.Now().After(e.expiry) {
if !ok || !c.clock.Now().Before(e.expiry) {
return nil, false
}
return e.val, true
Expand Down Expand Up @@ -148,7 +148,7 @@ func (c *Expiring) gc(now time.Time) {
// from looking at the (*expiringHeap).Pop() implmentation below.
// heap.Pop() swaps the first entry with the last entry of the heap, then
// calls (*expiringHeap).Pop() which returns the last element.
if len(c.heap) == 0 || now.After(c.heap[0].expiry) {
if len(c.heap) == 0 || now.Before(c.heap[0].expiry) {
return
}
cleanup := heap.Pop(&c.heap).(*expiringHeapEntry)
Expand Down
101 changes: 101 additions & 0 deletions staging/src/k8s.io/apimachinery/pkg/util/cache/expiring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,107 @@ func TestExpiration(t *testing.T) {
}
}

func TestGarbageCollection(t *testing.T) {
fc := &utilclock.FakeClock{}

type entry struct {
key, val string
ttl time.Duration
}

tests := []struct {
name string
now time.Time
set []entry
want map[string]string
}{
{
name: "two entries just set",
now: fc.Now().Add(0 * time.Second),
set: []entry{
{"a", "aa", 1 * time.Second},
{"b", "bb", 2 * time.Second},
},
want: map[string]string{
"a": "aa",
"b": "bb",
},
},
{
name: "first entry expired now",
now: fc.Now().Add(1 * time.Second),
set: []entry{
{"a", "aa", 1 * time.Second},
{"b", "bb", 2 * time.Second},
},
want: map[string]string{
"b": "bb",
},
},
{
name: "first entry expired half a second ago",
now: fc.Now().Add(1500 * time.Millisecond),
set: []entry{
{"a", "aa", 1 * time.Second},
{"b", "bb", 2 * time.Second},
},
want: map[string]string{
"b": "bb",
},
},
{
name: "three entries weird order",
now: fc.Now().Add(1 * time.Second),
set: []entry{
{"c", "cc", 3 * time.Second},
{"a", "aa", 1 * time.Second},
{"b", "bb", 2 * time.Second},
},
want: map[string]string{
"b": "bb",
"c": "cc",
},
},
{
name: "expire multiple entries in one cycle",
now: fc.Now().Add(2500 * time.Millisecond),
set: []entry{
{"a", "aa", 1 * time.Second},
{"b", "bb", 2 * time.Second},
{"c", "cc", 3 * time.Second},
},
want: map[string]string{
"c": "cc",
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
c := NewExpiringWithClock(fc)
for _, e := range test.set {
c.Set(e.key, e.val, e.ttl)
}

c.gc(test.now)

for k, want := range test.want {
got, ok := c.Get(k)
if !ok {
t.Errorf("expected cache to have entry for key=%q but found none", k)
continue
}
if got != want {
t.Errorf("unexpected value for key=%q: got=%q, want=%q", k, got, want)
}
}
if got, want := c.Len(), len(test.want); got != want {
t.Errorf("unexpected cache size: got=%d, want=%d", got, want)
}
})
}
}

func BenchmarkExpiringCacheContention(b *testing.B) {
b.Run("evict_probablility=100%", func(b *testing.B) {
benchmarkExpiringCacheContention(b, 1)
Expand Down

0 comments on commit cc76b1a

Please sign in to comment.