Fix reference/memory leaks #24
Conversation
src/PythonQt.cpp
Outdated
| @@ -525,6 +536,7 @@ void PythonQtPrivate::createPythonQtClassWrapper(PythonQtClassInfo* info, const | |||
| PythonQtClassInfo* outerClassInfo = lookupClassInfoAndCreateIfNotPresent(outerClass); | |||
| outerClassInfo->addNestedClass(info); | |||
| } else { | |||
| Py_INCREF(pyobj); | |||
| PyModule_AddObject(pack, info->className(), pyobj); | |||
There was a problem hiding this comment.
PyModule_AddObject only decrements the reference count on success, so you should check the return code.
There was a problem hiding this comment.
Yes, I saw this, there are many more such calls and I thought I leave it up for the next round of fixing, but now I fixed all of them by adding a wrapper function to do the job.
src/PythonQt.cpp
Outdated
| if (PyModule_Check(object)) { | ||
| PyModule_AddObject(object, QStringToPythonCharPointer(name), _p->wrapQObject(qObject)); | ||
| Py_XINCREF(wrappedObject); | ||
| PyModule_AddObject(object, QStringToPythonCharPointer(name), wrappedObject); |
There was a problem hiding this comment.
PyModule_AddObject only decrements the reference count on success, so you should check the return code.
src/PythonQt.cpp
Outdated
| if (PyModule_Check(object)) { | ||
| PyModule_AddObject(object, QStringToPythonCharPointer(name), PythonQtConv::QVariantToPyObject(v)); | ||
| Py_XINCREF(value); | ||
| PyModule_AddObject(object, QStringToPythonCharPointer(name), value); |
There was a problem hiding this comment.
PyModule_AddObject only decrements the reference count on success, so you should check the return code.
src/PythonQt.cpp
Outdated
| PyObject* value = NULL; | ||
| PyObject* key = NULL; | ||
| static PyObject* qtSlots = PyString_FromString("_qtSlots"); | ||
| static PyObject* qtSlots = NULL; | ||
| if (!qtSlots) |
There was a problem hiding this comment.
Please use braces even for one-line blocks
src/PythonQtImporter.cpp
Outdated
| if (err != 0) { | ||
| Py_DECREF(code); | ||
| Py_DECREF(mod); | ||
| return NULL; | ||
| } | ||
| #endif | ||
| } | ||
| Py_DECREF(mod); |
There was a problem hiding this comment.
Is this correct? The documentation states that PyModule_AddModule returns a borrowed reference. But I see that Py_DECREF is also used on mod before, so it seems as if this is the correct way to do this...
There was a problem hiding this comment.
Yes, good catch, I probably wrongly assumed the existing code was correct but you are right, documentation says it returns a borrowed reference so I removed all the DECREFs.
build/python.prf
Outdated
| unix:QMAKE_CXXFLAGS += $$system(python$${PYTHON_VERSION}-config --includes) | ||
| } | ||
| unix:QMAKE_CXXFLAGS += $$system(python$${PYTHON_VERSION}-config --includes) | ||
| unix:QMAKE_LFLAGS += $$system(python$${PYTHON_VERSION}-config --ldflags) |
There was a problem hiding this comment.
I had to add this line to build against Python-3.8.5 on my OpenSuSE machine
|
I tried to address the issues you showed up, please let me know if it's now fine to be committed. I wish I had the time to do a proper fixup/cleanup of PythonQt, but I found that Python itself cannot really clean up all the memory which surprised me, it's a nice language and I really like to use it for scripting my application, but I'll have to live with small leaks 😄 |
|
I have no objections to this merge request. Florian? |
|
Yes, looks good!
…On Mon 12. Oct 2020 at 15:40, Uwe Siems ***@***.***> wrote:
I have no objections to this merge request. Florian?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#24 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKHPHFWM6R3XOQMLMN3LLDSKMBLTANCNFSM4SKARYOQ>
.
|
|
Nice, thanks, will contribute again if I do more fixes! |
Tested with Python-3.8.5 debug build and memcheck, does not fix all leaks but some, reduces dynamic memory leaks by 2/3rds