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

ABC caches should use weak refs #46773

Closed
amauryfa opened this issue Mar 31, 2008 · 28 comments
Closed

ABC caches should use weak refs #46773

amauryfa opened this issue Mar 31, 2008 · 28 comments
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@amauryfa
Copy link
Member

BPO 2521
Nosy @amauryfa, @pitrou, @jackdied, @devdanzin, @benjaminp, @florentx
Dependencies
  • bpo-8268: Make old-style classes weak referenceable
  • Files
  • abc_leak.patch: Patch to fix the bug
  • leak_test.patch: Patch to add a test case
  • leak_test2.patch: Patch to add a test case, version 2
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2010-08-21.03:03:50.856>
    created_at = <Date 2008-03-31.15:12:29.847>
    labels = ['library', 'performance']
    title = 'ABC caches should use weak refs'
    updated_at = <Date 2011-05-13.17:43:32.186>
    user = 'https://github.com/amauryfa'

    bugs.python.org fields:

    activity = <Date 2011-05-13.17:43:32.186>
    actor = 'amaury.forgeotdarc'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-08-21.03:03:50.856>
    closer = 'benjamin.peterson'
    components = ['Library (Lib)']
    creation = <Date 2008-03-31.15:12:29.847>
    creator = 'amaury.forgeotdarc'
    dependencies = ['8268']
    files = ['16713', '16714', '18567']
    hgrepos = []
    issue_num = 2521
    keywords = ['patch']
    message_count = 28.0
    messages = ['64784', '87720', '101907', '101960', '101961', '101962', '101963', '101964', '101965', '101967', '102036', '102037', '102047', '104287', '104290', '105931', '105934', '110801', '112983', '112991', '113485', '114093', '114226', '114233', '114476', '114477', '135931', '135935']
    nosy_count = 9.0
    nosy_names = ['amaury.forgeotdarc', 'pitrou', 'jackdied', 'ajaksu2', 'benjamin.peterson', 'stutzbach', 'flox', 'BreamoreBoy', 'bluag']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue2521'
    versions = ['Python 2.7']

    @amauryfa
    Copy link
    Member Author

    The following function seems to 8 references each time it is run:

    import io, gc
    def f():
       class C: pass
       c=C()
       assert isinstance(c, io.StringIO) is False
       gc.collect();gc.collect();gc.collect()

    This is because io.StringIO._abc_negative_cache contains a strong
    reference to each "class C", as soon as isinstance() is called. These
    are never released.

    Python3.0 does use WeakSet for these caches, and does not leak.
    This is the (deep) reason why test_io.IOTest.test_destructor() leaks in
    the following report:
    http://mail.python.org/pipermail/python-checkins/2008-March/067918.html
    (The test derives from io.FileIO, and it is the call to the base class'
    method which calls isinstance() and put the class in the cache)

    @amauryfa amauryfa added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Mar 31, 2008
    @devdanzin
    Copy link
    Mannequin

    devdanzin mannequin commented May 13, 2009

    Confirmed on release26-maint and trunk.

    @devdanzin devdanzin mannequin added the performance Performance or resource usage label May 13, 2009
    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 29, 2010

    Now that WeakSet has been backported to trunk, I've backported the fix for this reference-leak (patch with test case attached).

    However, after making the patch, I discovered that old-style classes are not weak-referenceable. Consequently, the patch is not suitable for commiting.

    If old-style classes can be made weak-referenceable, then the patch should work. If not, then this bug is essentially impossible to solve in 2.x unless the ABC cache is done away with entirely.

    Other notes:

    Since abc.py is imported during bootstrapping before paths are set up, I needed to add _weakref as a built-in module. (It's already a built-in module in the py3k branch.)

    Since the patch tweaks Modules/Setup.dist, you need to "make distclean" and rerun ./configure after applying the patch.

    Also, on unpatched trunk the example posted by Amaury no longer seems to trigger the issue. However, the example I posted in bpo-8022 still triggers the leak. I used that as the basis for the test case in the patch.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 30, 2010

    Hmm, Benjamin pointed out that ABCs only support new-style classes, so old-style classes could be detected early instead of being added to the _abc_negative_cache.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 30, 2010

    By the way, Daniel, your patch doesn't look right.
    First, you shouldn't need all the sortedlist/sortedset hierarchy.
    Second, len(gc.get_objects()) is a truly horrible way of checking the classes have been destroyed. Just take a weakref to the class before deleting it, and check that calling the weakref returns None.
    (besides, we also have a refleak-detection run (regrtest -R) which can detect leaks even if you don't check them explicitly in your tests)

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 30, 2010

    I hadn't realized that old style classes didn't support ABCs. That certainly simplifies things! I'm working on a new patch.

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 30, 2010

    Are you sure the old-style classes don't support ABCs?

    ABCTestCase.validate_isinstance in Lib/test/test_collection.py specifically tests that both new-style and old-style classes work, unless I'm reading it wrong.

    (and those tests fail if I make ABCMeta.__instancecheck__ and ABCMeta.__subclasscheck__ always return False for old-style classes)

    @benjaminp
    Copy link
    Contributor

    In those cases, it's because __subclasscheck__ is overridden. You can't register a old-style class.

    @florentx
    Copy link
    Mannequin

    florentx mannequin commented Mar 31, 2010

    ABCTestCase.validate_isinstance ... specifically tests that
    both new-style and old-style classes work...

    I fixed some parts with issue bpo-7624, and changeset r78800.

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 31, 2010

    You can't register an old-style class, but many ABCs support duck-typing by implementing __subclasshook__. ABCMeta caches those results and stores a reference to old-style classes, sometimes in _abc_cache and sometimes in _abc_negative_cache. It can't simply return False.

    I guess that leaves two options:

    1. Make old-style classes weak-referenceable and cache the results of __subclasshook__.
    2. Refuse to cache old-style classes and call __subclasshook__ every time for old-style classes.
    Python 2.7a4+ (trunk:79493, Mar 30 2010, 19:19:13)
    >>> class old_iterable_class:
    ...   def __iter__(self):
    ...     pass
    ...
    >>> import collections
    >>> issubclass(old_iterable_class, collections.Iterable)
    True

    @pitrou
    Copy link
    Member

    pitrou commented Mar 31, 2010

    Make old-style classes weak-referenceable

    Now done (r79535).

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 31, 2010

    Cool! I will revise the patch based on your comments about my test case.

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Mar 31, 2010

    New patches uploaded. I separated out the patch to add the test case, to make it easier to test before and after applying the fix.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 26, 2010

    Someone with a better knowledge of ABCs than me should probably do a final review of this.

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Apr 26, 2010

    Someone with appropriate permissions want to update the "Stage"?

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented May 17, 2010

    Antoine, do you have a suggestion for someone with with better knowledge of ABCs to do the final review, so that I may very politely pester them? ;-)

    @pitrou
    Copy link
    Member

    pitrou commented May 17, 2010

    Antoine, do you have a suggestion for someone with with better
    knowledge of ABCs to do the final review, so that I may very politely
    pester them? ;-)

    I guess Benjamin could.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 19, 2010

    Patched the unit test, then ran the test before applying the fix which failed, after applying the fix the test ran successfully. Tested on Windows Vista 32 bit against 2.7 maintainance release. The patches are short and sweet, I see no reason why they can't go forward.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Aug 5, 2010

    Can a committer take a look at this please.

    @jackdied
    Copy link
    Contributor

    jackdied commented Aug 5, 2010

    This is a change in the codepath for instances that don't have __class__ defined.
             subclass = getattr(instance, '__class__', None)
    -        if subclass in cls._abc_cache:
    +        if subclass is not None and subclass in cls._abc_cache:

    I think the same thing happens in either case (from visual inspection of the code) but I'd rather not change it if we don't need to.

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Aug 9, 2010

    Jack,

    The change is necessary because "None in WeakSet()" would throw an exception.

    @benjaminp
    Copy link
    Contributor

    Some comments:

    • The test should be in test_abc.py and should probably not use a collections class.
    • Use test_support.gc_collect().
    • What's the __len__() stuff for?

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Aug 18, 2010

    Benjamin,

    Thanks for the feedback. Since you only commented on the test case, may I assume that the fix itself looked good to you?

    I will work on revising the test case based on your comments. Since I ran into the bug while working with the ABCs in the collections module, that biased my thinking when writing the test. :-)

    I needed to define__len__ because it's an abstract method in the ABC I used in the test (collections.Sized). I found that overriding it again in a sub-sub-class and calling it were necessary to trigger all of the ABC machinery leading to the leak.

    @stutzbach stutzbach mannequin added stdlib Python modules in the Lib dir and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 18, 2010
    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Aug 18, 2010

    Attached is a new test case, based on Benjamin's comments.

    @benjaminp
    Copy link
    Contributor

    r84230

    @stutzbach
    Copy link
    Mannequin

    stutzbach mannequin commented Aug 21, 2010

    Thanks! :-)

    @bluag
    Copy link
    Mannequin

    bluag mannequin commented May 13, 2011

    ImportError: No module named _weakrefset
    Here are some references while i was trying to install Pinax framework.

    http://paste.pound-python.org/show/6536/

    And i saw that the _weakrefset.py is not included in the package. So I have copied from Python's source with version : 3.1.* to my d:\sosyal\ folder. and everything works fine.

    @amauryfa
    Copy link
    Member Author

    bluag: the script you run contains a list of some modules required to start Python
    (I found a copy here: https://github.com/pinax/pinax/raw/master/scripts/pinax-boot.py )
    This script is obviously out of date wrt the new version of Python. Please report this to the pinax project!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants