cache: fix bug where TTLs were ignored leading to leaked memory in client agents #9978
Conversation
This mostly affects streaming since streaming will immediately return from Fetch calls when the state is Closed on eviction which causes the race condition every time. However this also affects all other cache types if the fetch call happens to return between the eviction and then next time around the Get loop by any client. There is a separate bug that allows cache items to be evicted even when there are active clients which is the trigger here.
banks
added
theme/streaming
| @@ -723,6 +723,22 @@ func (c *Cache) fetch(key string, r getOptions, allowNew bool, attempt uint, ign | |||
| // Set our entry | |||
| c.entriesLock.Lock() | |||
|
|
|||
| if _, ok := c.entries[key]; !ok { | |||
There was a problem hiding this comment.
I wonder if it is possible for a new entry to be added to the entries map while the fetch is happening. In that case we would still leak a goroutine right? We could compare the entry here, maybe by using the Expiry which is a pointer.
| if _, ok := c.entries[key]; !ok { | |
| if cachedEntry, ok := c.entries[key]; !ok || cachedEntry.Expiry != entry.Expiry { |
Probably not a big deal since the follow up PR makes this effectively impossible.
There was a problem hiding this comment.
🤔 I think if we get a double race and the entry expires and then is legitimately recreated by a new client Get during fetch then the behavior is still correct (i.e. not leaky) because by definition the client that recreated the entry is still interested in keeping it alive right? So it's not really being leaked in that case but rather kept because there is a client that cares about it still?
But maybe I'm missing a possible edge case?
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/347134. |
|
🍒✅ Cherry pick of commit ee04d45 onto |
…ient agents (#9978) * Fix bug in cache where TTLs are effectively ignored This mostly affects streaming since streaming will immediately return from Fetch calls when the state is Closed on eviction which causes the race condition every time. However this also affects all other cache types if the fetch call happens to return between the eviction and then next time around the Get loop by any client. There is a separate bug that allows cache items to be evicted even when there are active clients which is the trigger here. * Add changelog entry * Update .changelog/9978.txt
While testing Consul at scale we found this issue where agent caches might never expire.
It occurs for cache types that cancel requests and so return from
FetchwhenCloseis called.Currently, only streaming cache types have this behaviour but it is a supported behaviour for other future cache types too. The race here occurs very frequently (i.e. virtually all the time):
Closeis called on the cache type.Fetchcall to return.fetchmethod and immediately re-inserts the resulting error into the cache entry that was just removed, touches the TTL and lets it live again!Never removing the cache type even after clients have stopped requesting the data effectively causes a permanent memory leak which could be exacerbated by heavy churn in watches or tokens used during a client agent's lifetime.
Even if we move streaming types to not use the cache, it's worth fixing this so that
Closebehaviour is safe to use in future cache types without this leak.