wlav / cppyy

Other
391 stars 40 forks source link

__destruct__ not called back #111

Closed avarga closed 1 year ago

avarga commented 1 year ago

The documentation briefly mentions __destruct__, and I now o.__destruct__() works for explicitly calling the destructor from Python. However, it does not seem to be possible to use __destruct__ to override and extend the C++ destructor. Is the following code supposed to work?

import cppyy
cppyy.cppdef("""
#include <iostream>
class Foo { public: virtual ~Foo() { std::cout << "~Foo called" << std::endl; } };
""")

class Bar(cppyy.gbl.Foo):
    def __destruct__(self): 
        print("__destruct__ called")

b = Bar()
b = None  # --> prints "~Foo called", but not "__destruct__ called"
torokati44 commented 1 year ago

Is this related to #100 perhaps? If so, could the change in git mentioned under that also help with this?

wlav commented 1 year ago

It's a naming thing. __destruct__ is just a way of having access to the C++ destructor if there ever is a reason (there usually isn't, but since Python does not guarantee that destructors are called, it's sometimes useful).

The Python "destructor" is called __del__, so you want:

class Bar(cppyy.gbl.Foo):
    def __del__(self):
        print("__destruct__ called")

And again, there's no guarantee that __del__ is called on b = None (although it will in a simple case like this), b/c the frame may hold a reference, the garbage collector can schedule it later, it's part of program shutdown and it's a global variable of a module going down in round 2, etc., etc.

avarga commented 1 year ago

Let me explain the issue. Currently it seems that cppyy allows any C++ virtual member function to be overridden from Python, except for the (virtual) destructor. I am not sure why this distinction is made, at least I am not aware of any technical detail that would make overriding the destructor difficult or impossible. It would be very nice to be able to override a C++ virtual destructor from Python, from the completeness point of view, and also for the use case in my project.

Let me explain the background of why this is important for our project. I and @torokati44 are working on the widely used C++-based OMNeT++ simulation framework. In OMNeT++, components of the simulation are normally programmed in C++, and are instantiated and destroyed by the C++ simulation kernel. We are currently working on allowing our users to write simulation components in Python as well. Users would subclass from framework C++ classes in Python.

There are certain cleanup tasks we need to do in the component class' destructor, so if the component class is written in Python, there has to be a way to override the destructor in Python. __del__ doesn't cut it, because the object is deleted by the C++ simulation kernel, not by Python code.

import cppyy
cppyy.cppdef("""
#include <iostream>
class Foo { public: virtual ~Foo() {} };
void delete_Foo(Foo *foo) {delete foo;} // emulate how the simulation kernel destroys the component object
""")

class Bar(cppyy.gbl.Foo):
    def __destruct__(self): 
        print("Bar.__destruct__ called")

bar = Bar()
bar.__python_owns_ = False # to avoid crashing after the following line
cppyy.gbl.delete_Foo(bar)  # THIS LINE SHOULD PRINT "Bar.__destruct__ called"
bar = None
wlav commented 1 year ago

In the above code __del__ will be called, just not on cppyy.gbl.delete_Foo(bar), but on bar = None. Or, if bar is held in C++ (i.e. if a pointer to it is stored), __del__ will be called if bar = None preceeds C++-side deletion. It is not possible to destroy an object to which Python still has a ref-count w/o creating havoc on the interpreter side. It's done in another framework using an undocumented hook, but the ticket there is to replace the type of the Python object on the C++ side. That way, any further operations can all raise a ReferenceError. (Yes, it's a terrible hack, and only works on CPython.)

All that said, it's not technically hard to call __destruct__ (if any) from the C++ destructor of the dispatcher (I don't think calling __del__ is a good idea, as it'd be called twice). However, it needs to be understood that that in turn does not necessarily play nice with continuing to use the object Python side : __destruct__ needs to do an amount of cleanup in such a way that the Python object on any further use Python-side raises clear exceptions. Since it's probably sufficient of a niche case, that may be fine.

Also, this bar.__python_owns_ = False shouldn't be necessary, I think (since the C++ dispatcher destructor can nullify the proxy). I'll check as to why that is, as I expected that to already be the case.

avarga commented 1 year ago

I'm not familiar with cppyy internals, but the C++ dispatcher destructor nullifying the pointer in the proxy (and thus turning it into a "zombie") looks like a perfectly reasonable thing to do. All later attempts to use the Python proxy would result in an exception. This would allow deleting Python-created C++ objects from C++ code, even if there are Python references to it. A few years back I did a heavily-customized Swig-based Java language binding for OMNeT++ where I did the same, it worked out fine.

As a side note, I understand the way we use cppyy in OMNeT++ (a completely mixed-language program with C++ having the main control flow, as opposed to the more "normal" cppyy usage where a C++ class library is wrapped for Python users) somewhat stretches the limits, but perhaps that could also be a positive thing.

wlav commented 1 year ago

Both issues fixed in repo.

torokati44 commented 1 year ago

Thank you very much, can confirm the fix works!

avarga commented 1 year ago

Great, thank you for the fix!