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

Slots class holding a reference to the original version #407

Closed
gmacon opened this Issue Jul 11, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@gmacon
Contributor

gmacon commented Jul 11, 2018

I have a class hierarchy implemented with slots enabled, like this:

import attr

@attr.s(slots=True)
class BaseClass(object):
    foo = attr.ib(default='foo')

@attr.s(slots=True)
class SubClass(BaseClass):
    bar = attr.ib(default='bar')

This works fine until I wanted to introspect the class hierarchy:

BaseClass.__subclasses__()  # returns [<class '__main__.SubClass'>, <class '__main__.SubClass'>]

One of these is the original class, and the other is the new one created and returned by attr.s. This behavior manifests on both Python 3 and Python 2 (3.7.0 and 2.7.14, to be specific).

My understanding was that classes kept weak references to their subclasses to allow the __subclasses__ method to work, and I know that attrs creates and returns a new class when slots=True, but it's not clear to me why the old class stays around. I don't see any obvious place where a strong reference to it is held.

I guessed that there might be a reference cycle somewhere, so I tried adding a call to gc.collect() and checking gc.garbage, but that turned out to be incorrect.

Because I didn't see that the reference-counting mentioned in this comment was ever addressed, I also guessed that this might be a leak due to the __class__ closure cell fixup. I decided that this isn't the cause, either, because Python 2 does not have the __class__ closure cell.

@hynek

This comment has been minimized.

Member

hynek commented Jul 14, 2018

When replacing the class in slots=True attrs has to make sure that the class hierarchy remains intact, therefore the returned class is a subclass of the original one. Does that answer your question?

@gmacon

This comment has been minimized.

Contributor

gmacon commented Jul 14, 2018

That would answer the question, but I'm not convinced that it's true...

Python 3.6.6 (default, Jun 27 2018, 13:11:40)
[GCC 8.1.1 20180531] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import attr
>>> @attr.s(slots=True)
... class Foo: pass
...
>>> class Bar(Foo): pass
...
>>> AttrsBar = attr.s(slots=True)(Bar)
>>> issubclass(AttrsBar, Bar)
False
@hynek

This comment has been minimized.

Member

hynek commented Jul 15, 2018

Hm that might be due to how we create the class (by calling type() IIRC). Because OTOH:

>>> import attr

>>> class C: pass

>>> C2 = attr.s(slots=True)(C)

>>> C2.__mro__
(<class '__main__.C'>, <class 'object'>)

So the class is definitely there, it’s just that Python subclass machinery doesn’t know about it.

@gmacon

This comment has been minimized.

Contributor

gmacon commented Jul 15, 2018

I think you're being misled by the fact that the class generated by attrs still thinks its name is C despite being bound to C2:

>>> import attr
>>> class C: pass
...
>>> C2 = attr.s(slots=True)(C)
>>> C2
<class '__main__.C'>
>>> C2.__mro__[0] is C
False
>>> C2.__mro__[0] is C2
True

When the class is created, it's using the original classes bases as the new bases, and I didn't see anywhere that the bases get modified...

@hynek

This comment has been minimized.

Member

hynek commented Jul 17, 2018

Ah you’re right, turns out we’re more elaborate than I remembered:

cls = type(self._cls)(self._cls.__name__, self._cls.__bases__, cd)

So I guess if we have a leak, _ClassBuilder._create_slots_class() would be the place to look? Have you tried something like https://mg.pov.lt/objgraph/?

@gmacon

This comment has been minimized.

Contributor

gmacon commented Jul 17, 2018

Ah, no I hadn't thought of that...

attrs_slots_subclasses

I think that means the correct fix is to add __weakref__ to the exception list here:

attrs/src/attr/_make.py

Lines 493 to 497 in 35f7745

cd = {
k: v
for k, v in iteritems(self._cls_dict)
if k not in tuple(self._attr_names) + ("__dict__",)
}

With that:

>>> import attr
>>> @attr.s(slots=True)
... class C: pass
...
>>> @attr.s(slots=True)
... class C2(C): pass
...
>>> import gc
>>> gc.collect()
11
>>> C.__subclasses__()
[<class '__main__.C2'>]

The original class is left as a cyclic isolate when attr.s returns. I think that's as far as the attrs project needs to go to fix this; I'll call gc.collect() from my application. I'll write a test and open a PR today or tomorrow.

gmacon added a commit to gmacon/attrs that referenced this issue Jul 18, 2018

Do not copy __weakref__ from original _cls_dict
self._cls_dict["__weakref__"] holds a reference to self._cls, preventing
self._cls from being released after the new, slots-enabled class is
returned.

Fixes python-attrs#407

@gmacon gmacon referenced this issue Jul 18, 2018

Merged

Do not copy __weakref__ from original _cls_dict #410

3 of 7 tasks complete

@hynek hynek closed this in #410 Jul 28, 2018

hynek added a commit that referenced this issue Jul 28, 2018

Do not copy __weakref__ from original _cls_dict (#410)
self._cls_dict["__weakref__"] holds a reference to self._cls, preventing
self._cls from being released after the new, slots-enabled class is
returned.

Fixes #407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment