Skip to content

Store permanent values in the cache (for #1683)#1685

Merged
hawkw merged 5 commits intoeliza/generalize-cachefrom
ver/eliza/generalize-cache
May 19, 2022
Merged

Store permanent values in the cache (for #1683)#1685
hawkw merged 5 commits intoeliza/generalize-cachefrom
ver/eliza/generalize-cache

Conversation

@olix0r
Copy link
Member

@olix0r olix0r commented May 19, 2022

In some cases--like inbound policy discovery--we have a set of permanent
values that should never be removed from the cache. This change updates
the cache to only hold a handle when eviction is scheduled. No handle is
held when the cache entry is permanent.

This change also fixes a possible race condition. Previously, the
eviction task could consume an entry's handle without locking the cache.
This made it possible for a get_or_insert_with call to race against
the eviction task, adding an entry that would be immediately evicted by
the eviction task.

Signed-off-by: Oliver Gould [email protected]

In some cases--like inbound policy discovery--we have a set of permanent
values that should never be removed from the cache. This change updates
the cache to only hold a handle when eviction is scheduled. No handle is
held when the cache entry is permanent.

This change also fixes a possible race condition. Previously, the
eviction task could consume an entry's handle without locking the cache.
This made it possible for a `get_or_insert_with` call to race against
the eviction task, adding an entry that would be immediately evicted by
the eviction task.

Signed-off-by: Oliver Gould <[email protected]>
@olix0r olix0r requested a review from a team as a code owner May 19, 2022 02:29
@hawkw
Copy link
Contributor

hawkw commented May 19, 2022

Oh, whoops, I missed this and made my own almost-identical version of the permanent entry part of the change: 3525055

I'm going to merge this anyway to pick up the race fix --- good catch.

@hawkw hawkw merged commit 3098136 into eliza/generalize-cache May 19, 2022
@hawkw hawkw deleted the ver/eliza/generalize-cache branch May 19, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants