[vtk-developers] Patch for vtkPythonUtil.cxx

[Chris Kruszynski]

On 05-08-08 19:49, Prabhu Ramachandran wrote:
> Chris Kruszynski wrote:
>> I have observed this problem some time ago, it would actually need a 
>> second patch to fix all occurrences (or just another one?), which I 
>> have attached.
>>
>> The test case is easy:
> [...]
>
> Excellent!  I can confirm that this bug is present and your patch 
> fixes the problem.  I'll check this in along with the test case and 
> your description below.  Thanks.
>
>> This should cause a crash upon termination (trying to delete python 
>> object after interpreter shutdown), or an assertion error if the 
>> observed event is 'DeleteEvent' or 'AnyEvent' (trying to invoke 
>> python method after interpreter shutdown). The test case basically 
>> consists of a python class which contains a vtk object which is being 
>> observed by that same class. There is no error if the event handler 
>> is a member of a different class, even if the instance of that class 
>> is contained by the same python object which contains the vtk object. 
>> There is also no error if a vtk object is subclassed in python and 
>> that subclass has an event handler for itself 
>> (self.AddObserver(Event, self.Handler)). Finally, the problem 
>> disappears if the container class has a cyclic reference to itself 
>> (self.CyclicReference = self).
>
> This explains why I never saw this particular bug.  We didn't have any 
> observers on the same class for this would actually create a cyclic 
> reference and this reference cycle cannot be garbage collected!  This 
> is unlike the case when you have self.CyclicReference = self.
>
>> The problem can actually be pretty widespread since it affects all 
>> python classes which both contain and observe a vtk object.
>
> Indeed.
>
>> Before the changes to reference counting in revision 1.84 of 
>> vtkPythonUtil.cxx, the problem was masked, because an extra reference 
>> to the vtk object would remain somewhere, and the vtk object would 
>> never be deleted (and thus RemoveAllObservers would never be called, 
>> and vtkPythonCommand would never try to delete the python callable 
>> object after interpreter shutdown).
>
> I'm not so sure about this.  The reason is that even with older 
> versions of VTK you could do this:
>
> import vtk
>
> def f(o, e):
>     print e
>
> a = vtk.vtkObject()
> a.AddObserver('DeleteEvent', f)
> del a
>
> This prints "DeleteEvent", which means that 'a' was indeed deleted.  
> So it wasn't as if there was a memory leak.
>
> In addition, even when you add a cyclic reference, Py_IsInitialized is 
> still 0 and the destructor is still called.  So adding the reference 
> cycle did not prevent the vtkPythonHashDelete from being called.  If 
> vtkPythonCommand::~vtkPythonCommand is called in both cases, why is it 
> crashing in one case and not in another?
>
> If you notice, the crash is always in Py_Finalize() the documentation 
> for which has a nice section on this:
>
> """Bugs and caveats: The destruction of modules and objects in modules 
> is done in random order; this may cause destructors (__del__() 
> methods) to fail when they depend on other objects (even functions) or 
> modules. Dynamically loaded extension modules loaded by Python are not 
> unloaded. Small amounts of memory allocated by the Python interpreter 
> may not be freed (if you find a leak, please report it). Memory tied 
> up in circular references between objects is not freed. Some memory 
> allocated by extension modules may not be freed. Some extensions may 
> not work properly if their initialization routine is called more than 
> once; this can happen if an application calls Py_Initialize() and 
> Py_Finalize() more than once. """
>
> In our case the VTK objects are created from extension modules which 
> means their memory isn't freed at the time Py_Finalize is called.
> So the problem seems to be that if there is a collectable reference 
> cycle then the memory of the object is not freed, which explains why 
> the self.CyclicReference case works.  Some memory is freed, so it 
> looks like if the reference cycle is not detected, the memory is freed 
> leading to our bug when we deallocate the memory for this->obj twice 
> in the decref call.  I haven't dug deeper into the Python internals to 
> verify this though so it may be something else entirely.  My brief 
> reading of this suggests that there are dark areas of Py_Finalize that 
> I'd rather not get into ATM.
>
> Anyway, I'll check in your fix shortly.
I have dug deeper into why the crashes occur while they shouldn't, and I 
have uncovered the cause: the vtkPythonHash is deleted from 
vtkPythonHashDelete(), which is a Py_AtExit callback. The documentation 
for Py_AtExit is very clear:

"Since Python's internal finalization will have completed before the 
cleanup function, no Python APIs should be called by func. "

But what happens if python doesn't delete a cycle upon finalizing? The 
vtk object remains in the hash, and is deleted in the Py_AtExit 
callback. Now if the vtk object has a reference to python objects 
(callbacks), it is possible that during destruction Python API calls are 
used, which can lead to the crashes we see.

This affects both vtkPythonCommand as well as 
vtkPythonVoidFunc/vtkPythonVoidFuncArgDelete, which is I guess what 
Charl was talking about.

Python does seem to support garbage collection of cyclic references with 
a whole bunch of extra API calls, but this might require quite some 
effort to implement, and I don't think it should have a terribly high 
priority.

I have attached a fix for Charl's vtkPythonVoidFunc, the test case is:

import vtk

class TestCase2 :
 def __init__(self) :
   self.Filter = vtk.vtkProgrammableFilter()
   self.Filter.SetExecuteMethod(self.ExecMethod)
 def ExecMethod(self) :
   print 'execute method called'

test2 = TestCase2()

This one does not get fixed by self.Cycle = self, by the way. It ought 
to work for any place where SetExecuteMethod is wrapped, not just 
vtkProgrammableFilter, since they all use the same code.


Kind regards,

- Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20080806/64812250/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixpythonvoidfunc.diff
Type: text/x-patch
Size: 1190 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20080806/64812250/attachment-0001.bin>