Repository: allegro/bigcache
Pull request #219 (2020-05-04 14:53:03)

Deleted entries should not be removed by expire eviction
Hi, this is a small fix for #126,

I think the problem is that `shard.delete()` marks entries in queue as deleted ( by zero-ing all the 64 header bits for hash ), but `removeOldestEntry` is not checking for this case.

codecov-io: # [Codecov](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=h1) Report
> Merging [#219](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=desc) into [master](https://codecov.io/gh/allegro/bigcache/commit/37b9eb28693b36c88824afefa1dfe1a20826e74b&el=desc) will **decrease** coverage by `0.95%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/allegro/bigcache/pull/219/graphs/tree.svg?width=650&height=150&src=pr&token=NRx0gXD6he)](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
- Coverage   90.61%   89.66%   -0.96%     
==========================================
  Files          15       15              
  Lines         714      716       +2     
==========================================
- Hits          647      642       -5     
- Misses         58       63       +5     
- Partials        9       11       +2     
```


| [Impacted Files](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [shard.go](https://codecov.io/gh/allegro/bigcache/pull/219/diff?src=pr&el=tree#diff-c2hhcmQuZ28=) | `88.36% <100.00%> (-2.95%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=footer). Last update [37b9eb2...ae731ff](https://codecov.io/gh/allegro/bigcache/pull/219?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/shard.go b/shard.go
index 54eb65e..5256302 100644
--- a/shard.go
+++ b/shard.go
@@ -292,6 +292,10 @@ func (s *cacheShard) removeOldestEntry(reason RemoveReason) error {
 	oldest, err := s.entries.Pop()
 	if err == nil {
 		hash := readHashFromEntry(oldest)
+		if hash == 0 {
+			// entry has been explicitly deleted with resetKeyFromEntry, ignore
+			return nil
+		}
 		delete(s.hashmap, hash)
 		s.onRemove(oldest, reason)
 		if s.statsEnabled {


Test patch:
diff --git a/bigcache_test.go b/bigcache_test.go
index 8a04060..183614d 100644
--- a/bigcache_test.go
+++ b/bigcache_test.go
@@ -353,6 +353,67 @@ func TestOnRemoveFilter(t *testing.T) {
 	assertEqual(t, true, onRemoveInvoked)
 }
 
+func TestOnRemoveFilterExpired(t *testing.T) {
+	// t.Parallel()
+
+	// given
+	clock := mockedClock{value: 0}
+	onRemoveDeleted, onRemoveExpired := false, false
+	var err error
+	onRemove := func(key string, entry []byte, reason RemoveReason) {
+		switch reason {
+
+		case Deleted:
+			onRemoveDeleted = true
+		case Expired:
+			onRemoveExpired = true
+
+		}
+	}
+	c := Config{
+		Shards:             1,
+		LifeWindow:         3 * time.Second,
+		CleanWindow:        0,
+		MaxEntriesInWindow: 10,
+		MaxEntrySize:       256,
+		OnRemoveWithReason: onRemove,
+	}
+
+	cache, err := newBigCache(c, &clock)
+	assertEqual(t, err, nil)
+
+	// case 1: key is deleted AFTER expire
+	// when
+	onRemoveDeleted, onRemoveExpired = false, false
+	clock.set(0)
+
+	cache.Set("key", []byte("value"))
+	clock.set(5)
+	cache.cleanUp(uint64(clock.epoch()))
+
+	err = cache.Delete("key")
+
+	// then
+	assertEqual(t, err, ErrEntryNotFound)
+	assertEqual(t, false, onRemoveDeleted)
+	assertEqual(t, true, onRemoveExpired)
+
+	// case 1: key is deleted BEFORE expire
+	// when
+	onRemoveDeleted, onRemoveExpired = false, false
+	clock.set(0)
+
+	cache.Set("key2", []byte("value2"))
+	err = cache.Delete("key2")
+	clock.set(5)
+	cache.cleanUp(uint64(clock.epoch()))
+	// then
+
+	assertEqual(t, err, nil)
+	assertEqual(t, true, onRemoveDeleted)
+	assertEqual(t, false, onRemoveExpired)
+}
+
 func TestOnRemoveGetEntryStats(t *testing.T) {
 	t.Parallel()