Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that ActiveContextCache only enters a given combination of (activeCtx, localCtx) once in its internal FIFO #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kacarstensen-shift
Copy link

If ActiveContextCache.set is called multiple times during processing with a particular combination of activeCtx and localCtx, ActiveContextCache.order gets out of sync with ActiveContextCache.cache (i.e., multiple copies of a particular key combination can exist in order even though only one copy can be cached at any given time). This causes issues later in the cache's lifecycle when it begins to evict previously set elements. The first instance of a given (activeCtx, localCtx) found in ActiveContextCache.order will be evicted from ActiveContextCache.cache successfully, but subsequent copies will cause in a KeyError. To resolve this, I made inclusion in ActiveContextCache.order conditional on the (activeCtx, localCtx) combination not already existing in ActiveContextCache.cache. This keeps the two data structures are in sync and prevents the KeyErrors.

If we do this, then we'll try and fail to evict the same key combination
multiple times, leading to KeyErrors.
@Panaetius
Copy link

I'm surprised this PR has been open for this long.

I ran into the same issue, though I fixed it by changing set to:

def set(self, active_ctx, local_ctx, result):
        if len(self.order) == self.size:
            entry = self.order.popleft()
            if sum(
                e['activeCtx'] == entry['activeCtx'] and
                e['localCtx'] == entry['localCtx'] for e in self.order
            ) == 0:
                # only delete from cache if it doesn't exist in context deque
                del self.cache[entry['activeCtx']][entry['localCtx']]
        key1 = json.dumps(active_ctx)
        key2 = json.dumps(local_ctx)
        self.order.append({'activeCtx': key1, 'localCtx': key2})
        self.cache.setdefault(key1, {})[key2] = json.loads(json.dumps(result))

So only remove an entry from cache if it's not anywhere in the deque, this way contexts don't get evicted from cache if they're still used somewhere else, but the deque itself is still limited to self.size number of entries.

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