Crash in PyQt/Python3 app on exit

[Phil Thompson]

Sorry, but the first thing to do is upgrade to current versions of SIP 
and PyQt5.

Phil

On 09/01/2023 19:28, Kevin Constantine wrote:
> Hello!
> 
> 
> 
> We have a C++/Qt application that provides both a C++ api and a SIP 
> wrapped
> Python api.  When converting everything from Python 2.7 to Python 3, we
> began to see crashes when python applications using our api 
> exited/quit.  I
> have been tracking this crash down, and I think I have a decent
> understanding of what's causing it, which I'll attempt to explain 
> below.
> However, I don't feel like I have a good understanding of why the code 
> path
> that leads to the crash does what it does.  In other words, I'm not 
> sure if
> it's expected behavior, or a bug.  That's what I'm hoping you might be 
> able
> to assist with.
> 
> 
> 
> I have a simple-ish reproducer:
> 
> https://github.com/kevinconstantine/pyqt-crash
> 
> that demonstrates the problem if needed. If it's a bug, I have a patch 
> that
> prevents the crash, but given my current understanding of what's going 
> on,
> it may not be the right direction.
> 
> 
> 
> Versions
> 
> Python: 3.7.7
> 
> Qt: 5.15.2
> 
> PyQt: 5.15.4 (problem likely exists in 6.4.0 as well given code
> similarities)
> 
> sip: 4.19.30
> 
> 
> 
> Summary
> 
> It looks like in Python3, PyQt has registered a call-back when the
> application exits (cleanup_on_exit()).  The call chain eventually 
> results in
> sip_api_visit_wrapper() looping over SIP objects and calling PyQt's
> cleanup_qobject().  cleanup_qobject() checks if the object is owned by
> Python and checks if the object is a QObject.  If the object is owned 
> by
> C++ or not a QObject, it returns early.  Once we've established that 
> the
> object is owned by Python, it looks like it transfers ownership of the
> Python sipWrapper object to C++, and then calls delete on the address 
> of
> the sipWrapper object.
> 
> 
> 
> This delete causes the C++ destructors to get called, however, the SIP
> destructor is never called.  I would expect that a python owned object
> would have that SIP destructor called.  We are relying on doing some
> cleanup inside of the SIP destructor, and because that doesn't get
> executed, the application crashes.
> 
> 
> 
> 
> 
> More detail on what we're doing
> 
> For years, we've leveraged a couple of examples (here
> <https://riverbankcomputing.com/pipermail/pyqt/2005-March/010031.html>, 
> and
> here
> <https://www.riverbankcomputing.com/pipermail/pyqt/2017-September/039548.html>)
> to handle tracking shared pointers on the C++ side of things as they 
> got
> created through Python.  We have a hand-coded SIP destructor that gets
> called from Python that removes the shared pointer from a map so that 
> it
> too gets destructed properly.  This all worked great until we tried to
> migrate to Python 3 (cue the ominous music).
> 
> 
> 
> What's happening (at least my understanding of what's happening)
> 
> In Python3, PyQt has registered cleanup_on_exit() with
> sipRegisterExitNotifier()
> in qpy/QtCore/qpycore_init.cpp.  So when the application is exiting,
> cleanup_on_exit() is called and the call-stack looks like:
> 
> 
> 
> PyQt: cleanup_on_exit()
> 
>   PyQt: pyqt5_cleanup_qobjects()
> 
>     SIP: sip_api_visit_wrappers() // Loop over sip objects
> 
>       PyQt: cleanup_qobject()
> 
> 
> 
> Within qpy/QtCore/qpycore_public_api.cpp:cleanup_qobject(), several 
> checks
> are made that cause the function to return early without doing 
> anything:
> 
> 1. Anything not owned by Python returns early
> 
> 2. Non QObjects return early
> 
> 3. Running threads return early
> 
> 
> 
> But if an object passes all of these checks, the code calls 
> sipTransferTo()
> to transfer ownership of the sipWrapper object to C++, and then calls
> delete on the address of the sipWrapper object.
> 
> 
> 
> This delete causes the C++ destructors to get called, but most 
> importantly,
> the SIP destructor that we're relying on, is never called.  As 
> mentioned
> earlier, we are relying on the SIP destructor to clean up the global
> shared_ptr storage when the Python object is destroyed.  My expectation 
> is
> that a Python owned object would have that SIP destructor called just 
> like
> the SIP constructor is called when the object is created.
> 
> 
> 
> I added debugging output to SIP, PyQt, and my codebase in an effort to
> understand what was happening.  I'm hesitant to include that in this
> initial email as it gets kind of dense trying to explain what's going 
> on,
> but I'm happy to provide it if I can interest anyone in going down the
> rabbit hole with me on this one.
> 
> 
> 
> I also have a small patch that "fixes" my crash, but I'm not sure that 
> I
> completely have the right understanding of all of these pieces and that
> this doesn’t cause an issue elsewhere.
> 
> 
> 
> --- PyQt5-5.15.4.vanilla/qpy/QtCore/qpycore_public_api.cpp 2021-03-05
> 01:57:14.957003000 -0800
> 
> +++ PyQt5-5.15.4.kcc/qpy/QtCore/qpycore_public_api.cpp 2022-12-15
> 08:40:06.644173000 -0800
> 
> @@ -60,6 +60,13 @@
> 
>              return;
> 
>      }
> 
> 
> 
> +    // Try to destroy the object if it still has a reference
> 
> +    // Goal is to get the SIP dtor to fire.
> 
> +    if (Py_REFCNT((PyObject *)sw)) {
> 
> +        sipInstanceDestroyed(sw);
> 
> +        return;
> 
> +    }
> 
> +
> 
>      sipTransferTo((PyObject *)sw, SIP_NULLPTR);
> 
> 
> 
>      Py_BEGIN_ALLOW_THREADS
> 
> 
> 
> By calling sipInstanceDestroyed(), the SIP destructor fires, and
> subsequently the C++ dtors get executed as well.  Everything appears to 
> get
> cleaned up in the proper order, and we avoid the crash.  I have not
> reproduced the issue in PyQt6, but there are no differences in
> cleanup_qobject() between the two versions, so I'm assuming the 
> behavior is
> similar.
> 
> 
> 
> Thanks so much for any help you can provide in fixing this, or helping 
> me
> understand better what's going on.
> 
> 
> 
> -kevin