Fix LruCache callback deduplication

[V02460]

The PR fixes the deduplication issues for memoizing caches. It makes all identical _CacheContext objects share the same invalidate function. This is required for proper deduplication of invalidate callbacks in LruCache. LruCache is used for the @cached function decorator. Fixes #6200.

LruCache relies on the equality of callback functions for deduplication. This is in contrast to what was annotated in _CacheContext which claims that LruCache relies on the equality of _CacheContext objects itself.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file
• Pull request includes a sign off

[richvdh]

yup, so this has been changed by python/cpython@ac20e0f#diff-8bd563e1946aef62abb51ecc6d162eaf.

[richvdh]

thank you very much for the contribution!

This is in contrast to what was annotated in _CacheContext which claims that LruCache relies on the equality of _CacheContext objects itself.

well, it was kind of true, in that the equality of the invalidate methods depended on the equality of the _CacheContext objects, though I agree it wasn't very clear.

Anyway: I think this might be more intuitively solved by ensuring that we don't create duplicate _CacheContext objects, so:

• leave _CacheContext as it was
• create in CacheDescriptor.__get__, do cache_contexts = {}
• at line 433, do kwargs["cache_context"] = cache_contexts.setdefault(cache_key, _CacheContext(cache, cache_key))

wdyt?

[V02460]

Ensuring _CacheContext objects are not duplicated seems saner to me as well, right.

I'd argue that it's not CacheDescriptor's responsibility to manage caching of _CacheContext instances. Because of this and to not clutter CacheDescriptor even more, it might be better to put that code into a get_instance factory method of _CacheContext instead.

I think we still need to use a WeakValueDictionary instead of a normal dict as you suggested. For every new argument of a method call a _CacheContext object is created, so these become many and really should be released when no longer used.

[richvdh]

I'd argue that it's not CacheDescriptor's responsibility to manage caching of _CacheContext instances. Because of this and to not clutter CacheDescriptor even more, it might be better to put that code into a get_instance factory method of _CacheContext instead.

ok, that sounds reasonable.

I think we still need to use a WeakValueDictionary instead of a normal dict as you suggested. For every new argument of a method call a _CacheContext object is created, so these become many and really should be released when no longer used.

I think you're right. I hadn't quite thought that part through!

[richvdh]

lgtm. I've merged latest develop into the branch to try to get the CI to sort itself out.

[richvdh]

🎉

[richvdh]

thank you very much for contributing this and for working through the pain from the CI!

[auscompgeek]

FYI you didn't have to turn the function annotations into comments - that's valid Python 3 syntax.

[V02460]

Oh thank you, I didn't know function annotation syntax was available earlier (PEP 3107, Python 3.0) than variable annotation syntax (PEP 526, Python 3.6).