Fixup memoization key_factory bugs.

Review Request #2317 — Created June 2, 2015 and submitted

jsirois
pants
jsirois/memo/harden
1632
d9d89f4...
pants-reviews
benjyw, patricklaw, zundel
Patrick pointed out 2 issues in 4ee47c085380a8175e5c2d1795e9f367809cda
that can lead to bad cache keys and thus incorrect memoized function
return values.

This adds a failing test for the ambiguous *args/**kwargs case and fixes
it.  The GC'd object `id()` case is not tested/testable.

 src/python/pants/util/memo.py             | 16 +++++++++++++---
 tests/python/pants_test/util/test_memo.py | 14 ++++++++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)
CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/65258867
  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
JS
ZU
  1. I don't see any issues, but then I didn't see any with the previous RB either.

  2. src/python/pants/util/memo.py (Diff revision 1)
     
     

    typo, s/instace/instance/

  3. 
      
PA
  1. 
      
  2. src/python/pants/util/memo.py (Diff revision 1)
     
     

    Clever

    1. Idea stolen from twitter.common.decorators.lru_cache which is stolen from python3 lru_cache.
  3. src/python/pants/util/memo.py (Diff revision 1)
     
     
  4. src/python/pants/util/memo.py (Diff revision 1)
     
     

    This is now a memory leak.

    1. I think this can be mitigated with this: https://docs.python.org/2/library/weakref.html

    2. But even using weak refs, how does it behave on immutable keys? Keys that are objects but tend to be recreated on the fly just to be a key?

      I don't think there's a generalized way to do this correctly that doesn't put a lot of burden on the user to know the exact semantics of the decorator, which kind of defeats the purpose. I'll be happy to upstream our standard cached_property implementation today if you're okay with it.

    3. I'm OK with it - I'll submit this to patch the 2 bugs and your commit can delete the whole thing.
      
      I'll note 2 things though:
      + _by design_ this was already a memory leak and its been spelled out clearly in the function doc.
      + clearly python core thoought something like this was useable-enough to implement despite tradeoffs.  They seal in the key factory and cache impl though (@lru_cache: https://docs.python.org/3.3/library/functools.html#functools.lru_cache).
  5. 
      
JS
JS
JS
  1. Thanks guys - submitted @ https://github.com/pantsbuild/pants/commit/26115df8659183ec69b505071e8402f70674dbf6
    
    I'll stay hands-off on this bit of code so Patrick can proceed with an un-interrupted revert & replace with `cached_property`.
  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...