Issue1634167

__del__ does not appear to work correctly if acquired in jython.  This py script works fine on Python 2.5:


class z:
   pass

a = z()
def __del__(a):
    print "DEL"
z.__del__ = __del__
a = None

x = z()
x = None

print dir(z)

class z:
    def __del__(a):
        print "DEL2"
f = z()
f = None

import sys
if(len(sys.argv) > 1):
    import java
    java.lang.System.gc()
    java.lang.Thread.sleep(5000)


z always has its __del__ called even if acquired.  In jython this appears not to work for classes that acquire __del__.  Im not sure of the cause, but I would expect the instance created after acquiring __del__ to fire "DEL" off.  I believe the problem for the second case is because PyClass does not mutate when __del__ is added to it.  Well it mutates but it doesn't appear to reset an internal variable that determines if the new PyInstance will be finalizable or not.  Additionally it appears that PyType defines a variable that is not used: "needs_finalizable".  PyClass looks for __del__ being set not "needs_finalizable".

Now for existing classes this poses a problem.  If __del__ is defined at instantiation time it is a PyFinalizableInstance that is created.  The documentation states that PyFinalizableInstance exists because of performance problems.  It would be good to see what these are.  The only solution I can think of at this moment is to remove PyFinalizableInstance and just make PyInstance with a finalize method.  It could then check on finalize if __del__ exists and do its work.

A possible fix for the __del__ added after the class is defined is the patch attached.  If an instance is created after this it will be able to create a PyFinalizableInstance.  Now this patch in itself seem problematic.  PyFinalizableInstance doesn't check if its __del__ method has been nulled out.  But this doesn't appear to cause any terrible problems at finalize time.  It still would be better to check things out and not throw a NullPointerException(this appears hidden).

The simplest fix would be to define finalize in PyInstance and check if __del__ is not null in its instclass variable.  If so, execute __del__.  This would probably take care of any problems with mutations on __del__ for all existing instances.  Though I am worried about PyInstances sitting in memory longer than they need to.


File Added: PyClassDiff.txt

There may be more mutation problems beyond __del__:
    // Store these methods for performance optimization

    // These are only used by PyInstance

    PyObject __getattr__, __setattr__, __delattr__, __tojava__, __del__,

            __contains__;


from "PyClass".  If these are mutated I don't see any corresponding change code in the PyClass instance that will reset these fields.

----------------------

A possible different solution to the mutating __del__ problem is to use a variation on the FinalizerGuardian from Effective Java.
1. Keep track of all PyInstances created by the PyClass
2. If __del__ is altered iterate the PyInstances and add/delete a FinalizerGuardian whose responsibility is to __del__ the PyInstance when itself is finalised.

This is good in that it makes the PyInstance finalizability changable on the fly.  Its bad because its going to add alot of references to the system and a special object for finalising.

Im not sure if I consider this a definative patch for this problem.  This one is a half-measure.  Its better than before but if there is going to be a real fix, it needs to be more radical.

When I converted PyClass to exposed annotations I fixed the problem 
solved by this match, of it not resetting its cached version of values 
like __del__ and __getattr__

I've noticed that PyPy seems to emit a RuntimeWarning to the user if 
they apply a __del__ to a type after the fact. Since I don't see this 
happening very often, that sounds like an ok solution for objects that 
didn't have a __del__ in the first place

Though I haven't seen the FinalizerGuardian technique Leo's refering to, 
it's probably worth looking into

Maybe we don't even need to emit a warning, since we can support adding a __del__ to a type, it's just that the destructor will only work for objects created after the destructor was attached. We just can't support pre-existing instances of that type all of aquiring a __del__

Maybe just documenting this fact somewhere would be enough

This issue has been open for 5 years now. Does it need to remain open? Is there a simple resolution?

This looks closable to me since we have mostly fixed it. Philip, if you think I'm wrong feel free to re-open. I want to get the number of outstanding bugs with "patch" selected down.