zope.app.interface PersistentInterfaceClass dependents broken

Bug #98474 reported by Ross Patterson
2
Affects Status Importance Assigned to Milestone
Zope 3
Status tracked in 3.4
3.2
Fix Committed
Undecided
Unassigned
3.3
Fix Committed
Undecided
Unassigned
3.4
Fix Released
Medium
Unassigned

Bug Description

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.

Revision history for this message
Ross Patterson (rossp) wrote :
Revision history for this message
Ross Patterson (rossp) wrote :

Uploaded: persistent-interfaces-fix.diff

a fix for all but the zope.interface.interface.InterfaceClass provides declaration weak reference bug

Revision history for this message
Ross Patterson (rossp) wrote :

I forgot that I have commit priveleges on svn.zope.org. All tests pass. Shall I just commit this?

Revision history for this message
Ross Patterson (rossp) wrote :

I committed this in SVN r72109. I removed the unrelated failing weakref test so as not to commit a failing test. I'll open another issue with a patch to add that test back in. This one can be closed as far as I can tell.

Revision history for this message
Christian Theune (ctheune) wrote :

Marked fixed for 3.4. Backports should be applied still.

Revision history for this message
Ross Patterson (rossp) wrote :

Does that mean I should merge this into 3.3 and/or 3.2? I'd be happy to do so.

Revision history for this message
Christian Theune (ctheune) wrote : Re: [Bug 98474] Re: zope.app.interface PersistentInterfaceClass dependents broken

Am Montag, den 13.08.2007, 01:39 +0000 schrieb Ross Patterson:
> Does that mean I should merge this into 3.3 and/or 3.2? I'd be happy to
> do so.

Yes, please. :)

Revision history for this message
Ross Patterson (rossp) wrote :

Fix has already been applied

Revision history for this message
Ross Patterson (rossp) wrote :

Backported the changes

Revision history for this message
Ross Patterson (rossp) wrote :

Christian Theune <email address hidden> writes:

> Am Montag, den 13.08.2007, 01:39 +0000 schrieb Ross Patterson:
>> Does that mean I should merge this into 3.3 and/or 3.2? I'd be happy to
>> do so.
>
> Yes, please. :)

Sorry about the *long* delay, but these fixes have finally been
backported to the 3.2 and 3.3 branches and the zope.app.interface tests
pass under both.

I noted the changes in launchpad.

Ross

Revision history for this message
Philipp von Weitershausen (philikon) wrote :

Setting status to "Fix released" for the the 3.4 line, we've just had another 3.4.0 beta (3.4.0b2) which contains the fix.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.