[x/cache] Fix LRU cache mem leak (when used with no loader) #3806

linasm · 2021-10-02T10:11:45Z

What this PR does / why we need it:
A regression in LRU cache was introduced here:

Lines 356 to 366 in e2c6903

    
           if getWithNoLoader && !exists { 
        
           	// If we're not using a loader then return entry not found 
        
           	// rather than creating a loading channel since we are not trying 
        
           	// to load an element we are just attempting to retrieve it if and 
        
           	// only if it exists. 
        
           	return nil, false, nil, ErrEntryNotFound 
        
           } 
        
           if !exists { 
        
           	// The entry doesn't exist, clear enough space for it and then add it 
        
           	if err := c.reserveCapacity(1); err != nil {

Cache entries were never being evicted when it was being used with no loader. Because of early return under getWithNoLoader = true the code that invokes reserveCapacity was unreachable, and this was the only place where eviction would happen.

Special notes for your reviewer:
I have added a call to reserveCapacity from Put (which assumes no loader is in use).
Another possibility would be to fix tryCache implementation to reserve the space on cache miss, but then Put would happen not under the same lock, creating a race between Gets (freeing up space in advance) and Puts which would permit exceeding the cache limit (still being eventually consistent). I chose doing eviction from Put as this seemed more semantically correct.

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

codecov · 2021-10-02T10:31:52Z

Codecov Report

Merging #3806 (6b1a24d) into master (6b1a24d) will not change coverage.
The diff coverage is n/a.

❗ Current head 6b1a24d differs from pull request most recent head 6845b33. Consider uploading reports for the commit 6845b33 to get more accurate results

@@          Coverage Diff           @@
##           master   #3806   +/-   ##
======================================
  Coverage    56.8%   56.8%           
======================================
  Files         552     552           
  Lines       63077   63077           
======================================
  Hits        35883   35883           
  Misses      23996   23996           
  Partials     3198    3198

Flag	Coverage Δ
aggregator	`63.3% <0.0%> (ø)`
cluster	`∅ <0.0%> (∅)`
collector	`58.4% <0.0%> (ø)`
dbnode	`60.5% <0.0%> (ø)`
m3em	`46.4% <0.0%> (ø)`
metrics	`19.7% <0.0%> (ø)`
msg	`74.3% <0.0%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b1a24d...6845b33. Read the comment docs.

robskillington · 2021-10-02T14:20:42Z

src/x/cache/lru_cache.go

+		if enforceLimit && c.reserveCapacity(1) != nil {
+			// Silently skip adding the new entry if we fail to free up space for it
+			// (which should never be happening).
+			return value, err
+		}


Should return the error from the reserve capacity call yeah? Otherwise the err would be nil yeah?

Suggested change

if enforceLimit && c.reserveCapacity(1) != nil {

// Silently skip adding the new entry if we fail to free up space for it

// (which should never be happening).

return value, err

}

if enforceLimit {

if err := c.reserveCapacity(1); err != nil {

// Silently skip adding the new entry if we fail to free up space for it

// (which should never be happening).

return value, err

}

}

Well, this aspect is a bit vague. The contract of updateCacheEntryWithLock is to return its arguments value interface{}, err error unmodified, and I've tried to preserve that, commenting that we "silently skip" such situation (being unable to evict from cache - which from what I see would only happen in case there is a code bug).
Also, in the only place that calls updateCacheEntryWithLock with enforceLimit = true, those return values are ignored completely:

m3/src/x/cache/lru_cache.go

Lines 223 to 233 in c5f6237

func (c *LRU) PutWithTTL(key string, value interface{}, ttl time.Duration) {

var expiresAt time.Time

if ttl > 0 {

expiresAt = c.now().Add(ttl)

}

c.mut.Lock()

defer c.mut.Unlock()

_, _ = c.updateCacheEntryWithLock(key, expiresAt, value, nil, true)

}

So I'm not really sure if I should change this (but don't have a strong opinion either, even after jumping around this code for quire a long time). I guess that's the support of two modes (with/without loader) in a single data structure that makes it hard to come up with an elegant implementation. Perhaps @ryanhall07 will chime in with his insights as well.

I see yeah, that makes sense. Feel free to ignore my suggestion then 👍

The other question is, this only happens when there is a code bug yeah?

If so.. does that mean we have a remaining code bug we're not aware of? Which is fine if so, we can chase this up after the fact. Just wanted to understand current state of the world (post-merging this change).

No, what I meant, reserveCapacity is not supposed to return errors, unless there is some bug that we are not yet aware of.

Ah ok, that makes sense.

left another comment. maybe it's just best to change reserveCapacity to return a bool so it's clear it can't be an error.

robskillington

LGTM

ryanhall07 · 2021-10-04T14:44:16Z

src/x/cache/lru_cache.go

 ) (interface{}, error) {
 	entry := c.entries[key]
 	if entry == nil {
+		if enforceLimit && c.reserveCapacity(1) != nil {


nit: maybe check the returned error type is ErrCacheFull in case a future contributor changes reserveCapacity to return some other kind of error. or change the signature of reserveCapacity to return a bool instead to be more explicit it's not really an error.

Indeed, replacing return value error with bool for reserveCapacity makes lots of sense. Should have thought of this myself.

ryanhall07 · 2021-10-04T14:49:42Z

src/x/cache/lru_cache.go

+		if enforceLimit && c.reserveCapacity(1) != nil {
+			// Silently skip adding the new entry if we fail to free up space for it
+			// (which should never be happening).
+			return value, err
+		}


left another comment. maybe it's just best to change reserveCapacity to return a bool so it's clear it can't be an error.

ryanhall07 · 2021-10-04T14:52:31Z

src/x/cache/lru_cache.go

@@ -229,7 +229,7 @@ func (c *LRU) PutWithTTL(key string, value interface{}, ttl time.Duration) {
 	c.mut.Lock()
 	defer c.mut.Unlock()

-	_, _ = c.updateCacheEntryWithLock(key, expiresAt, value, nil)
+	_, _ = c.updateCacheEntryWithLock(key, expiresAt, value, nil, true)


i hate to over optimize, but I do wonder if scanning the entire cache for evictions on every put is going to be too much. I guess we can try this and if it causes performance issues, we can add some kind of eviction every N puts.

That should not be a problem - reserveCapacity removes entries while scanning them, so we either free up one slot for the current Put value, or free up more and then some subsequent Puts will be handled for free.

[x/cache] Fix LRU cache mem leak (when used with no loader)

f294aef

linasm requested review from ryanhall07 and robskillington October 2, 2021 10:11

robskillington reviewed Oct 2, 2021

View reviewed changes

linasm and others added 3 commits October 2, 2021 17:28

Also check size in property based test

c5f6237

Merge branch 'master' into linasm/fix-lru-cache-mem-leak-no-loader

712fd03

Merge branch 'master' into linasm/fix-lru-cache-mem-leak-no-loader

9f2f705

robskillington approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'master' into linasm/fix-lru-cache-mem-leak-no-loader

6845b33

ryanhall07 approved these changes Oct 4, 2021

View reviewed changes

linasm and others added 2 commits October 4, 2021 19:42

reserveCapacity returns a bool

790e89c

Merge branch 'master' into linasm/fix-lru-cache-mem-leak-no-loader

3aeab78

linasm merged commit 4a4559d into master Oct 4, 2021

linasm deleted the linasm/fix-lru-cache-mem-leak-no-loader branch October 4, 2021 17:24

wesleyk mentioned this pull request Nov 17, 2021

m3db node memory leak(v1.3) #3868

Closed

wisre mentioned this pull request Dec 9, 2021

the issue of m3db node mem leak is exist #3986

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[x/cache] Fix LRU cache mem leak (when used with no loader) #3806

[x/cache] Fix LRU cache mem leak (when used with no loader) #3806

linasm commented Oct 2, 2021 •

edited

Loading

codecov bot commented Oct 2, 2021 •

edited

Loading

robskillington Oct 2, 2021 •

edited

Loading

linasm Oct 2, 2021

robskillington Oct 2, 2021 •

edited

Loading

linasm Oct 2, 2021

robskillington Oct 4, 2021

ryanhall07 Oct 4, 2021

robskillington left a comment

ryanhall07 Oct 4, 2021

linasm Oct 4, 2021

ryanhall07 Oct 4, 2021

ryanhall07 Oct 4, 2021

linasm Oct 4, 2021

	if getWithNoLoader && !exists {
	// If we're not using a loader then return entry not found
	// rather than creating a loading channel since we are not trying
	// to load an element we are just attempting to retrieve it if and
	// only if it exists.
	return nil, false, nil, ErrEntryNotFound
	}

	if !exists {
	// The entry doesn't exist, clear enough space for it and then add it
	if err := c.reserveCapacity(1); err != nil {

	func (c *LRU) PutWithTTL(key string, value interface{}, ttl time.Duration) {
	var expiresAt time.Time
	if ttl > 0 {
	expiresAt = c.now().Add(ttl)
	}

	c.mut.Lock()
	defer c.mut.Unlock()

	_, _ = c.updateCacheEntryWithLock(key, expiresAt, value, nil, true)
	}

[x/cache] Fix LRU cache mem leak (when used with no loader) #3806

[x/cache] Fix LRU cache mem leak (when used with no loader) #3806

Conversation

linasm commented Oct 2, 2021 • edited Loading

codecov bot commented Oct 2, 2021 • edited Loading

Codecov Report

robskillington Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm commented Oct 2, 2021 •

edited

Loading

codecov bot commented Oct 2, 2021 •

edited

Loading

robskillington Oct 2, 2021 •

edited

Loading

robskillington Oct 2, 2021 •

edited

Loading