[Zope3-dev] PersistentInterfaceClass Fix - Issue 747

[Ross Patterson]

I've submitted some new tests and a fix for
zope.app.interface.PersistentInterfaceClass in:

http://www.zope.org/Collectors/Zope3-dev/747

I have commit priveleges to the svn repo, but I'd love to get some
review before commiting.  baijum from #zope3-dev suggested I post
here.  Below is a copy of the text from issue 747.

> zope.app.interface.PersistentInterfaceClass uses a PersistentDict for
> the dependents attribute instead of the WeakKeyDictionary used in
> zope.interface.interface.InterfaceClass. There are a number of bugs
> associated with this approach not exposed in any tests. Attached is a
> diff to the tests of zope.app.interface and zope.app.component to
> expose these bugs.

> Firstly, if IFoo is a zope.app.interface.PersistentInterfaceClass
> instance and zope.interface.directlyProvides(foo, IFoo), the
> declaraion can't be verified from a different ZODB connection with
> IFoo.providedBy(foo) due to the following. When the ZODB is trying to
> unpickle the dependents attribute of IFoo, it procedes down the
> serialization of IFoo to the ProvidesClass instance representing the
> declaration. It begins reconstituting the ProvidesClass instance,
> which calls IFoo.subscribe(provides) which accesses the dependents
> attribute of IFoo. Since the ProvidesClass instance isn't persistent,
> it has no oid the ZODB circular reference check doesn't catch this
> circle. As a result the fix is to make sure that the declarations
> instances in the dependents attribute are themselves persistent.

> Also attached is a diff to zope.app.interface that replaces
> PersistentInterfaceClass.dependents with a custom dict that converts
> non-persistent declarations being added to the dependents attribute
> into persistent versions of the same.

> This fixes the problem but is not optimal because there are then two
> instances for the same declaration, one in the ProvidesClass instance
> stored in the object's __provides__ attribute and the other in the
> dependents attribute of the PersistentInterfaceClass.

> It seems like the more appropriate solution would be to check for
> PersistentInterfaceClass instances in
> zope.interface.declarations.directlyProvides and use the persistent
> declaration classes for those declaraions. Since
> PersistentInterfaceClass is in zope.app.interface and zope.interface,
> I wasn't sure which was the lesser of the two evils, so I restrained
> my changes to zope.app. What might be a better solution? Give me
> some feedback and I'll change the implementation.

> There's another problem with the
> zope.app.interface.PersistentInterfaceClass.dependents attribute.
> zope.interface.InterfaceClass.dependents is a WeakKeyDictionary so
> that declarations don't keep objects from being freed if the object is
> removed. By using a PersistentDict for dependents, the declarations
> can keep an object from being freed from memory and/or the ZODB when
> the object is removed.

> My first patch also includes tests for this bug. These tests seem to
> have exposed another unrelated bug. An instance of a class that
> subclasses persistent.Persistent is declared that it
> zope.interface.directlyProvides a zope.interface.InterfaceClass
> instance and then the persistent object is added to the ZODB and
> committed. Then if the persistent instance is deleted from the ZODB,
> the transaction is committed, and the ZODB is packed, and gc.collect
> is run, the ProvidesClass instance in the InterfaceClass instance
> still remains. It does not, however, remain if the persistent
> interface was never added to the ZODB.

> I'm not sure if this represents a potential memory leak or not. What
> confuses me is that it all behaves properly unless the persistent
> instance is added to the ZODB. I noted the comment about weak
> referrences being added to the ZODB optomistically in ZODB.serialize.
> Could that be it? Unfortunately, my second patch doesn't include a
> fix for this. I'd be happy to investigate this further if given a
> little direction. 

Ross