3.8.0: dictionary changed size during iteration iterating on importlib_metadata.distributions() #293

Activity

1. m-melis commented on Mar 27, 2021

   

   m-melis

   on Mar 27, 2021

   Last edited by m-melis

   More actions

   Confirmed. All our CI pipeline unittests fail due to the new version: https://gitlab.com/secml/secml/-/pipelines/277431124

   This is independent of the Python version.
2. jaraco commented on Mar 27, 2021

   

   jaraco

   on Mar 27, 2021

   Member

   More actions

   Thanks for the report. I can't tell from the traceback what's happening, and the issue isn't happening in my CI environments that also rely on importlib_metadata 3.8 and pluggy. Moreover, when looking at #290, I don't see anything in particular where a dictionary size might be altered. There is this line, but the infos and eggs are mutated in the __init__ so shouldn't be iterated over before they're fully constructed.

   Can someone help distill the issue into a reproducer I can use to troubleshoot?
3. jaraco commented on Mar 27, 2021

   

   jaraco

   on Mar 27, 2021

   Member

   More actions

   I'm able to reproduce the issue by checking out secml and running tox -e py36.
4. jaraco commented on Mar 27, 2021

   

   jaraco

   on Mar 27, 2021

   Last edited by jaraco

   Member

   More actions

   If I disable the lru_cache around FastPath.lookup, the problem goes away:

   diff --git a/importlib_metadata/__init__.py b/importlib_metadata/__init__.py
   index 11b9aac..1b95c8f 100644
   --- a/importlib_metadata/__init__.py
   +++ b/importlib_metadata/__init__.py
   @@ -648,7 +648,6 @@ class FastPath:
        def mtime(self):
            with contextlib.suppress(OSError):
                return os.stat(self.root).st_mtime
   -        self.lookup.cache_clear()
    
        @method_cache
        def lookup(self, mtime):
   diff --git a/importlib_metadata/_functools.py b/importlib_metadata/_functools.py
   index 73f50d0..632292f 100644
   --- a/importlib_metadata/_functools.py
   +++ b/importlib_metadata/_functools.py
   @@ -75,7 +75,7 @@ def method_cache(method, cache_wrapper=None):
        def wrapper(self, *args, **kwargs):
            # it's the first call, replace the method with a cached, bound method
            bound_method = types.MethodType(method, self)
   -        cached_method = cache_wrapper(bound_method)
   +        cached_method = bound_method
            setattr(self, method.__name__, cached_method)
            return cached_method(*args, **kwargs)
    
5. jaraco commented on Mar 27, 2021

   

   jaraco

   on Mar 27, 2021

   Member

   More actions

   If I switch to a different method of memoization of that method, the problem remains:

   diff --git a/importlib_metadata/__init__.py b/importlib_metadata/__init__.py
   index 11b9aac..8649630 100644
   --- a/importlib_metadata/__init__.py
   +++ b/importlib_metadata/__init__.py
   @@ -21,7 +21,7 @@ from ._compat import (
        Protocol,
    )
    
   -from ._functools import method_cache
   +from ._functools import memoize
    from ._itertools import unique_everseen
    
    from configparser import ConfigParser
   @@ -648,9 +648,8 @@ class FastPath:
        def mtime(self):
            with contextlib.suppress(OSError):
                return os.stat(self.root).st_mtime
   -        self.lookup.cache_clear()
    
   -    @method_cache
   +    @memoize
        def lookup(self, mtime):
            return Lookup(self)
    
   diff --git a/importlib_metadata/_functools.py b/importlib_metadata/_functools.py
   index 73f50d0..f678ce0 100644
   --- a/importlib_metadata/_functools.py
   +++ b/importlib_metadata/_functools.py
   @@ -83,3 +83,44 @@ def method_cache(method, cache_wrapper=None):
        wrapper.cache_clear = lambda: None
    
        return wrapper
   +
   +
   +# from
   +# http://code.activestate.com/recipes/577452-a-memoize-decorator-for-instance-methods/
   +class memoize:
   +    """cache the return value of a method
   +
   +    This class is meant to be used as a decorator of methods. The return value
   +    from a given method invocation will be cached on the instance whose method
   +    was invoked. All arguments passed to a method decorated with memoize must
   +    be hashable.
   +
   +    If a memoized method is invoked directly on its class the result will not
   +    be cached. Instead the method will be invoked like a static method:
   +    class Obj(object):
   +        @memoize
   +        def add_to(self, arg):
   +            return self + arg
   +    Obj.add_to(1) # not enough arguments
   +    Obj.add_to(1, 2) # returns 3, result is not cached
   +    """
   +    def __init__(self, func):
   +        self.func = func
   +
   +    def __get__(self, obj, objtype=None):
   +        if obj is None:
   +            return self.func
   +        return functools.partial(self, obj)
   +
   +    def __call__(self, *args, **kw):
   +        obj = args[0]
   +        try:
   +            cache = obj.__cache
   +        except AttributeError:
   +            cache = obj.__cache = {}
   +        key = (self.func, args[1:], frozenset(kw.items()))
   +        try:
   +            res = cache[key]
   +        except KeyError:
   +            res = cache[key] = self.func(*args, **kw)
   +        return res

   That indicates that it's the memoization of the lookup object that's implicated in the failure.
6. omry commented on Mar 27, 2021

   

   omry

   on Mar 27, 2021

   Author

   More actions

   The only change between 3.7.3 and 3.8.0 is the caching (v3.7.3...v3.8.0).
   Can the content of the cached object somehow change after it's created?
7. jaraco commented on Mar 27, 2021

   

   jaraco

   on Mar 27, 2021

   Member

   More actions

   I've figured out the issue. It's in how Lookup.search uses lazy evaluation of self.info and how collections.defaultdict will add keys to the dict when none exist. So here's what happens:

   1. Pluggy searches for plugins and iterates over those plugins.
   2. In the midst of searching for plugins, Pluggy initializes plugins, including a plugin that imports jsonschema, which invokes importlib_metadata.version('jsonschema'), which invokes Lookup.search('jsonschema').
   3. The lookup of jsonschema, by dint of collections.defaultdict, creates a new entry in Lookup.infos, which is still being iterated over by Pluggy in 1.
8. jaraco

   added a commit that references this issue on Mar 27, 2021

   Eagerly consume infos and eggs. Workaround for #293. Still needs a test.

   d91dd25
9. omry commented on Mar 27, 2021

   

   omry

   on Mar 27, 2021

   Author

   More actions

   Gotcha.
   A solution can be to give a copy of the dict for users to iterate on.
   There is a tiny perf cost but it will should work around the problem.
10. jaraco commented on Mar 27, 2021

    

    jaraco

    on Mar 27, 2021

    Last edited by jaraco

    Member

    More actions

    I've got a workaround going out in 3.8.1. I still want to produce a test that captures the failure, but I wanted to get the fix out asap.

    👍React with 👍1Reacted by Simon Brugman
11. omry commented on Mar 27, 2021

    

    omry

    on Mar 27, 2021

    Author

    More actions

    I just tested your fix and it solves my problem.

    😄React with 😄1Reacted by Jason R. Coombs
12. jaraco commented on Mar 27, 2021

    

    jaraco

    on Mar 27, 2021

    Member

    More actions

    Sorry for the inconvenience.
13. jaraco

    added a commit that references this issue on Mar 27, 2021

    Add test capturing failure. Ref #293.

    f545a8f
14. omry commented on Mar 27, 2021

    

    omry

    on Mar 27, 2021

    Author

    More actions

    No worries, thanks for fixing it so quickly.
    Test looks good to me, I hope that's the last of this problem :).
15. jaraco

    closed this as completedon Mar 27, 2021