<div dir="ltr"><div class="gmail_default" style="font-family:courier new,monospace">Thanks Phil! I started to mock up something similar to what PyQt is doing where it registers a handler with atexit(). The issue that I'm running into currently is that PyQt's handler is getting called before mine, and PyQt is destroying these objects, so in my handler where I'm also trying to use <br>sipVisitWrappers(handler_func, QCoreApplication::instance());</div><div class="gmail_default" style="font-family:courier new,monospace">to iterate over the SIP objects so that I can try to call their destructors, or delete the underlying data, there's no longer anything to iterate over.<br></div><div class="gmail_default" style="font-family:courier new,monospace"><br></div><div class="gmail_default" style="font-family:courier new,monospace">Going back to my original question: when PyQt is cleaning things up, it's taking a Python owned object, and passing the ownership over to C++ and then destroying it. That causes the C++ destructor to get called, but not the SIP destructor. That transfer of ownership feels incorrect, but again, this isn't my domain, so I don't really know for sure.</div><div class="gmail_default" style="font-family:courier new,monospace"><br></div><div class="gmail_default" style="font-family:courier new,monospace">-kevin</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 26, 2023 at 11:54 AM Phil Thompson <<a href="mailto:phil@riverbankcomputing.com" target="_blank">phil@riverbankcomputing.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">SIP deliberately suppresses the invocation of Python reimplementations <br>
of C++ virtuals once the interpreter starts to exit. It registers a <br>
handler with atexit() to do this.<br>
<br>
I'd suggest you register your own tidy-up handler from within your <br>
bindings. This is guaranteed to be called before SIP's handler.<br>
<br>
Phil<br>
<br>
On 26/01/2023 15:34, Kevin Constantine wrote:<br>
> If test.py crashes, that's reproducing the problem.<br>
> <br>
> On Thu, Jan 26, 2023 at 10:31 AM Phil Thompson <br>
> <<a href="mailto:phil@riverbankcomputing.com" target="_blank">phil@riverbankcomputing.com</a>><br>
> wrote:<br>
> <br>
>> Using the current pre-release packages at...<br>
>> <br>
>> <a href="https://www.riverbankcomputing.com/pypi/simple/" rel="noreferrer" target="_blank">https://www.riverbankcomputing.com/pypi/simple/</a><br>
>> <br>
>> ...I can't reproduce the problem. test.py crashes and test-cleanup.py<br>
>> doesn't (as you are expecting).<br>
>> <br>
>> Phil<br>
>> <br>
>> On 25/01/2023 22:56, Kevin Constantine wrote:<br>
>> > Hey Phil-<br>
>> ><br>
>> > I have an environment set up with:<br>
>> > Sip: 6.7.5<br>
>> > PyQt5: 5.15.7<br>
>> > Qt: 5.15.2<br>
>> ><br>
>> > I'm still seeing the crash.<br>
>> ><br>
>> > I've updated the reproducer repository with the changes to support the<br>
>> > newer version of Sip: <a href="https://github.com/kevinconstantine/pyqt-crash" rel="noreferrer" target="_blank">https://github.com/kevinconstantine/pyqt-crash</a><br>
>> ><br>
>> > -kevin<br>
>> ><br>
>> > On Tue, Jan 10, 2023 at 12:44 PM Kevin Constantine <<br>
>> > <a href="mailto:kevin.constantine@disneyanimation.com" target="_blank">kevin.constantine@disneyanimation.com</a>> wrote:<br>
>> ><br>
>> >> Thanks Phil-<br>
>> >> I'll give it a whirl and report back.<br>
>> >><br>
>> >> -kevin<br>
>> >><br>
>> >> On Tue, Jan 10, 2023 at 12:13 PM Phil Thompson <<br>
>> >> <a href="mailto:phil@riverbankcomputing.com" target="_blank">phil@riverbankcomputing.com</a>> wrote:<br>
>> >><br>
>> >>> Sorry, but the first thing to do is upgrade to current versions of<br>
>> >>> SIP<br>
>> >>> and PyQt5.<br>
>> >>><br>
>> >>> Phil<br>
>> >>><br>
>> >>> On 09/01/2023 19:28, Kevin Constantine wrote:<br>
>> >>> > Hello!<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > We have a C++/Qt application that provides both a C++ api and a SIP<br>
>> >>> > wrapped<br>
>> >>> > Python api. When converting everything from Python 2.7 to Python 3,<br>
>> we<br>
>> >>> > began to see crashes when python applications using our api<br>
>> >>> > exited/quit. I<br>
>> >>> > have been tracking this crash down, and I think I have a decent<br>
>> >>> > understanding of what's causing it, which I'll attempt to explain<br>
>> >>> > below.<br>
>> >>> > However, I don't feel like I have a good understanding of why the<br>
>> code<br>
>> >>> > path<br>
>> >>> > that leads to the crash does what it does. In other words, I'm not<br>
>> >>> > sure if<br>
>> >>> > it's expected behavior, or a bug. That's what I'm hoping you might<br>
>> be<br>
>> >>> > able<br>
>> >>> > to assist with.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > I have a simple-ish reproducer:<br>
>> >>> ><br>
>> >>> > <a href="https://github.com/kevinconstantine/pyqt-crash" rel="noreferrer" target="_blank">https://github.com/kevinconstantine/pyqt-crash</a><br>
>> >>> ><br>
>> >>> > that demonstrates the problem if needed. If it's a bug, I have a<br>
>> patch<br>
>> >>> > that<br>
>> >>> > prevents the crash, but given my current understanding of what's<br>
>> going<br>
>> >>> > on,<br>
>> >>> > it may not be the right direction.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > Versions<br>
>> >>> ><br>
>> >>> > Python: 3.7.7<br>
>> >>> ><br>
>> >>> > Qt: 5.15.2<br>
>> >>> ><br>
>> >>> > PyQt: 5.15.4 (problem likely exists in 6.4.0 as well given code<br>
>> >>> > similarities)<br>
>> >>> ><br>
>> >>> > sip: 4.19.30<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > Summary<br>
>> >>> ><br>
>> >>> > It looks like in Python3, PyQt has registered a call-back when the<br>
>> >>> > application exits (cleanup_on_exit()). The call chain eventually<br>
>> >>> > results in<br>
>> >>> > sip_api_visit_wrapper() looping over SIP objects and calling PyQt's<br>
>> >>> > cleanup_qobject(). cleanup_qobject() checks if the object is owned<br>
>> by<br>
>> >>> > Python and checks if the object is a QObject. If the object is owned<br>
>> >>> > by<br>
>> >>> > C++ or not a QObject, it returns early. Once we've established that<br>
>> >>> > the<br>
>> >>> > object is owned by Python, it looks like it transfers ownership of<br>
>> the<br>
>> >>> > Python sipWrapper object to C++, and then calls delete on the address<br>
>> >>> > of<br>
>> >>> > the sipWrapper object.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > This delete causes the C++ destructors to get called, however, the<br>
>> SIP<br>
>> >>> > destructor is never called. I would expect that a python owned<br>
>> object<br>
>> >>> > would have that SIP destructor called. We are relying on doing some<br>
>> >>> > cleanup inside of the SIP destructor, and because that doesn't get<br>
>> >>> > executed, the application crashes.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > More detail on what we're doing<br>
>> >>> ><br>
>> >>> > For years, we've leveraged a couple of examples (here<br>
>> >>> > <<br>
>> <a href="https://riverbankcomputing.com/pipermail/pyqt/2005-March/010031.html" rel="noreferrer" target="_blank">https://riverbankcomputing.com/pipermail/pyqt/2005-March/010031.html</a>>,<br>
>> >>><br>
>> >>> > and<br>
>> >>> > here<br>
>> >>> > <<br>
>> >>><br>
>> <a href="https://www.riverbankcomputing.com/pipermail/pyqt/2017-September/039548.html" rel="noreferrer" target="_blank">https://www.riverbankcomputing.com/pipermail/pyqt/2017-September/039548.html</a><br>
>> >>> >)<br>
>> >>> > to handle tracking shared pointers on the C++ side of things as they<br>
>> >>> > got<br>
>> >>> > created through Python. We have a hand-coded SIP destructor that<br>
>> gets<br>
>> >>> > called from Python that removes the shared pointer from a map so that<br>
>> >>> > it<br>
>> >>> > too gets destructed properly. This all worked great until we tried<br>
>> to<br>
>> >>> > migrate to Python 3 (cue the ominous music).<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > What's happening (at least my understanding of what's happening)<br>
>> >>> ><br>
>> >>> > In Python3, PyQt has registered cleanup_on_exit() with<br>
>> >>> > sipRegisterExitNotifier()<br>
>> >>> > in qpy/QtCore/qpycore_init.cpp. So when the application is exiting,<br>
>> >>> > cleanup_on_exit() is called and the call-stack looks like:<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > PyQt: cleanup_on_exit()<br>
>> >>> ><br>
>> >>> > PyQt: pyqt5_cleanup_qobjects()<br>
>> >>> ><br>
>> >>> > SIP: sip_api_visit_wrappers() // Loop over sip objects<br>
>> >>> ><br>
>> >>> > PyQt: cleanup_qobject()<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > Within qpy/QtCore/qpycore_public_api.cpp:cleanup_qobject(), several<br>
>> >>> > checks<br>
>> >>> > are made that cause the function to return early without doing<br>
>> >>> > anything:<br>
>> >>> ><br>
>> >>> > 1. Anything not owned by Python returns early<br>
>> >>> ><br>
>> >>> > 2. Non QObjects return early<br>
>> >>> ><br>
>> >>> > 3. Running threads return early<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > But if an object passes all of these checks, the code calls<br>
>> >>> > sipTransferTo()<br>
>> >>> > to transfer ownership of the sipWrapper object to C++, and then calls<br>
>> >>> > delete on the address of the sipWrapper object.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > This delete causes the C++ destructors to get called, but most<br>
>> >>> > importantly,<br>
>> >>> > the SIP destructor that we're relying on, is never called. As<br>
>> >>> > mentioned<br>
>> >>> > earlier, we are relying on the SIP destructor to clean up the global<br>
>> >>> > shared_ptr storage when the Python object is destroyed. My<br>
>> expectation<br>
>> >>> > is<br>
>> >>> > that a Python owned object would have that SIP destructor called just<br>
>> >>> > like<br>
>> >>> > the SIP constructor is called when the object is created.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > I added debugging output to SIP, PyQt, and my codebase in an effort<br>
>> to<br>
>> >>> > understand what was happening. I'm hesitant to include that in this<br>
>> >>> > initial email as it gets kind of dense trying to explain what's going<br>
>> >>> > on,<br>
>> >>> > but I'm happy to provide it if I can interest anyone in going down<br>
>> the<br>
>> >>> > rabbit hole with me on this one.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > I also have a small patch that "fixes" my crash, but I'm not sure<br>
>> that<br>
>> >>> > I<br>
>> >>> > completely have the right understanding of all of these pieces and<br>
>> that<br>
>> >>> > this doesn’t cause an issue elsewhere.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > --- PyQt5-5.15.4.vanilla/qpy/QtCore/qpycore_public_api.cpp 2021-03-05<br>
>> >>> > 01:57:14.957003000 -0800<br>
>> >>> ><br>
>> >>> > +++ PyQt5-5.15.4.kcc/qpy/QtCore/qpycore_public_api.cpp 2022-12-15<br>
>> >>> > 08:40:06.644173000 -0800<br>
>> >>> ><br>
>> >>> > @@ -60,6 +60,13 @@<br>
>> >>> ><br>
>> >>> > return;<br>
>> >>> ><br>
>> >>> > }<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > + // Try to destroy the object if it still has a reference<br>
>> >>> ><br>
>> >>> > + // Goal is to get the SIP dtor to fire.<br>
>> >>> ><br>
>> >>> > + if (Py_REFCNT((PyObject *)sw)) {<br>
>> >>> ><br>
>> >>> > + sipInstanceDestroyed(sw);<br>
>> >>> ><br>
>> >>> > + return;<br>
>> >>> ><br>
>> >>> > + }<br>
>> >>> ><br>
>> >>> > +<br>
>> >>> ><br>
>> >>> > sipTransferTo((PyObject *)sw, SIP_NULLPTR);<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > Py_BEGIN_ALLOW_THREADS<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > By calling sipInstanceDestroyed(), the SIP destructor fires, and<br>
>> >>> > subsequently the C++ dtors get executed as well. Everything appears<br>
>> to<br>
>> >>> > get<br>
>> >>> > cleaned up in the proper order, and we avoid the crash. I have not<br>
>> >>> > reproduced the issue in PyQt6, but there are no differences in<br>
>> >>> > cleanup_qobject() between the two versions, so I'm assuming the<br>
>> >>> > behavior is<br>
>> >>> > similar.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > Thanks so much for any help you can provide in fixing this, or<br>
>> helping<br>
>> >>> > me<br>
>> >>> > understand better what's going on.<br>
>> >>> ><br>
>> >>> ><br>
>> >>> ><br>
>> >>> > -kevin<br>
>> >>><br>
>> >><br>
>> <br>
</blockquote></div>