fix: map ttl reinsert head or tail #14
Conversation
arthhhhh23
force-pushed
the
fix/ttl-map-orphaned-elements
branch
from
2422d9b to
0511b16
Compare
ttl_test.go
Outdated
| @@ -174,6 +176,121 @@ func TestTTLScenario(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestHeadTailLogicConcurrent(t *testing.T) { | |||
| m := NewMapTTLCache[string, string](context.Background(), time.Millisecond, time.Hour) | |||
There was a problem hiding this comment.
I usually use context with deferred cancel to avoid having live goroutine after test is finished (this sometimes causes test to fail, eg when using goleak).
| t.Errorf("unexpected error in cleanup: %v", err) | ||
| } | ||
|
|
||
| if m.Len() != 0 { |
There was a problem hiding this comment.
Should we also add a check that head and tail are zero values? Just in case.
There was a problem hiding this comment.
Oh now it found coverage decrease. I can add missing tests later.
| c.data[key] = val | ||
| return | ||
| } | ||
|
|
||
| // If it's already the tail, we only need to update the value and timestamp |
There was a problem hiding this comment.
Trying to wrap my head around what actually was happening before. Looks like the root issue is that we were overwriting tail with record that had prev = tail?
There was a problem hiding this comment.
I think what happened was when the set key is the head we did not update the head, leaving orphans.
- [A, B, C, D] where A is head, D is tail
- inserts A again, A becomes tail with no next but still is the head because it was not set to B, this orphans B,C,D as they are no longer part of the linked list
There was a problem hiding this comment.
Updating head should not be a common case when cache is heavily utilised. Updating tail will.
| rec := c.data[c.tail] | ||
| rec.timestamp = ts | ||
| rec.value = value | ||
| c.data[c.tail] = rec |
There was a problem hiding this comment.
nit: Why not just this?
| rec := c.data[c.tail] | |
| rec.timestamp = ts | |
| rec.value = value | |
| c.data[c.tail] = rec | |
| c.data[c.tail] = val |
There was a problem hiding this comment.
because val.prev is the element being inserted itself, so we're losing the link to the actual prev
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
===========================================
- Coverage 100.00% 99.17% -0.83%
===========================================
Files 7 7
Lines 661 729 +68
===========================================
+ Hits 661 723 +62
- Misses 0 4 +4
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. |
No description provided.