Skip to content

fix: LRUCache.get() and __getitem__() fail for cached None values#623

Open
koteshyelamati wants to merge 1 commit into
msiemens:masterfrom
koteshyelamati:master
Open

fix: LRUCache.get() and __getitem__() fail for cached None values#623
koteshyelamati wants to merge 1 commit into
msiemens:masterfrom
koteshyelamati:master

Conversation

@koteshyelamati

Copy link
Copy Markdown

Problem

LRUCache.get() and LRUCache.__getitem__() both break when None is a legitimate cached value.

get() bug: uses if value is not None: to detect cache hits, so if a key maps to None, it skips the LRU promotion and returns the default instead of the cached None.

__getitem__() bug: calls self.get(key) and then checks if value is None: raise KeyError, so it raises KeyError even when the key exists with value None.

cache = LRUCache(capacity=5)
cache['key'] = None

cache.get('key', 'missing')  # Returns 'missing' instead of None
cache['key']                  # Raises KeyError instead of returning None

Root cause

Both methods use the cached value itself as a sentinel to detect cache misses. This is the same class of bug that was fixed in set() by PR #597.

Fix

Replace the value-based checks with key not in self.cache membership tests, consistent with how set() was already fixed.

When a cached value is None, LRUCache.get() incorrectly returns the default
instead of the cached None, and LRUCache.__getitem__() raises KeyError even
though the key exists.

Root cause: both methods check `if value is not None` / `if value is None`
to detect cache misses, but this fails when None is a legitimate cached value.

Fix: check `if key not in self.cache` instead, mirroring the fix applied
to LRUCache.set() in PR msiemens#597.

Reproducer:
    cache = LRUCache(capacity=5)
    cache['key'] = None
    assert cache.get('key', 'default') == None  # fails before fix
    assert cache['key'] is None  # raises KeyError before fix
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.

1 participant