[Cython] Bad interaction between cimported types and module cleanup

[Lisandro Dalcin]

On 4 August 2012 17:50, Stefan Behnel <stefan_ml at behnel.de> wrote:
> Stefan Behnel, 04.08.2012 14:59:
>> Lisandro Dalcin, 03.08.2012 18:51:
>>> diff --git a/Cython/Compiler/ModuleNode.py b/Cython/Compiler/ModuleNode.py
>>> index 2472de3..255b047 100644
>>> --- a/Cython/Compiler/ModuleNode.py
>>> +++ b/Cython/Compiler/ModuleNode.py
>>> @@ -1111,7 +1111,11 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
>>>          if base_type:
>>>              tp_dealloc = TypeSlots.get_base_slot_function(scope, tp_slot)
>>>              if tp_dealloc is None:
>>> -                tp_dealloc = "%s->tp_dealloc" % base_type.typeptr_cname
>>> +                # Using the cimported base type pointer interacts
>>> +                # badly with module cleanup nullyfing these pointers.
>>> +                # Use instead the base type pointer from the
>>> +                # instance's type pointer.
>>> +                tp_dealloc = "Py_TYPE(o)->tp_base->tp_dealloc"
>>>              code.putln(
>>>                      "%s(o);" % tp_dealloc)
>>>          else:
>>
>> Tried it, doesn't work. The problem is that this always goes through the
>> actual type of the object, ignoring the type hierarchy completely, which
>> kills the tp_dealloc() call chain and runs into an infinite loop starting
>> from the second inheritance level (or a crash because of multiple DECREF
>> calls on the same fields).
>
> Here is a fix that should handle this correctly.
>
> https://github.com/cython/cython/commit/a8393fa58741c9ae0647e8fdec5fee4ffd91ddf9
>
> Basically, it checks the type pointer of cimported types on tp_traverse(),
> tp_clear() and tp_dealloc() and in the unlikely case that they are NULL, it
> walks the type hierarchy up to the point where it finds the current
> function and then calls the one in the base type.
>

Nice!

> It is somewhat involved, but I still doubt that it would lead to a real
> slow down. Maybe tp_traverse() is the one that's a bit more time critical
> than the others because it can be called more often, but the one additional
> NULL check in the normal case still shouldn't hurt all that much.
>

OK.

> It fixes the case in lxml for me, but I didn't shrink that down to a test
> case yet. Lisandro, if you find the time, it would be nice if you could
> write one up for the problem you ran into.
>

To properly test this we would need to namespace the __cleanup
functions registered with atexit, I mean to name them
'__<package>_<module>_cleanup', that way we could loop over registered
functions to and call them "by hand". I you agree, I can take care of
this and write some tests for module cleanup.



-- 
Lisandro Dalcin
---------------
CIMEC (INTEC/CONICET-UNL)
Predio CONICET-Santa Fe
Colectora RN 168 Km 472, Paraje El Pozo
3000 Santa Fe, Argentina
Tel: +54-342-4511594 (ext 1011)
Tel/Fax: +54-342-4511169