Yiling-J / theine-go

high performance in-memory cache
MIT License
252 stars 16 forks source link

fix of bug with removed keys in processDeque #30

Closed sivukhin closed 9 months ago

sivukhin commented 9 months ago

processDeque function has suspicious logic:

if deleted {
    removedkv = append(
        expiredkv, s.kvBuilder(evicted),
    )
    s.postDelete(evicted)
}

It looks like a typo as removedkv slice always reseted with expiredkv content in case of new evicted element.

Unfortunately, I didn't managed to implement test that checks this behaviour. I am ready to do that - but need some guidance from maintainers.

This PR change the logic to the following one: removedkv = append(removedkv, s.kvBuilder(evicted))

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c903895) 88.99% compared to head (d92ec2b) 88.58%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #30 +/- ## ========================================== - Coverage 88.99% 88.58% -0.41% ========================================== Files 24 24 Lines 2562 2559 -3 ========================================== - Hits 2280 2267 -13 - Misses 194 201 +7 - Partials 88 91 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Yiling-J commented 9 months ago

@sivukhin thanks for the fixing! this is definitely a bug. About testing, you can update TestProcessDeque like this:

func TestProcessDeque(t *testing.T) {
    store := NewStore[int, int](20000, false, nil, nil, nil, 0, 0)

    evicted := map[int]int{}
    store.removalListener = func(key, value int, reason RemoveReason) {
        if reason == EVICTED {
            evicted[key] = value
        }
    }
    _, index := store.index(123)
    shard := store.shards[index]
    shard.qsize = 10

    for i := 0; i < 5; i++ {
        entry := &Entry[int, int]{key: i}
        entry.cost.Store(1)
        store.shards[index].deque.PushFront(entry)
        store.shards[index].qlen += 1
        store.shards[index].hashmap[i] = entry
    }

    // move 0,1,2 entries to slru
    store.Set(123, 123, 8, 0)
    require.Equal(t, store.shards[index].deque.Len(), 3)
    keys := []int{}
    for store.shards[index].deque.Len() != 0 {
        e := store.shards[index].deque.PopBack()
        keys = append(keys, e.key)
    }
    require.Equal(t, []int{3, 4, 123}, keys)
    require.Equal(t, 0, len(evicted))

    // test evicted callback, cost less than threshold will be evicted immediately
    store.policy.threshold.Store(100)
    for i := 10; i < 15; i++ {
        entry := &Entry[int, int]{key: i}
        entry.cost.Store(1)
        store.shards[index].deque.PushFront(entry)
        store.shards[index].qlen += 1
        store.shards[index].hashmap[i] = entry
    }
    store.shards[index].mu.Lock()
    store.processDeque(store.shards[index])
    require.Equal(t, 5, len(evicted))

}
sivukhin commented 9 months ago

@Yiling-J, sure, thanks! Update the test as you said (and checked that before fix it was indeed red)