From 20d599b1ded55be7d5c765e79dc8a6d492b36cb8 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sat, 7 Sep 2024 21:27:50 +0200 Subject: [PATCH 1/3] Mute noisy diagnostics with Python debug builds, avoid post-Finalize Python API access by pre-Finalize cleanup of modules. This avoids an assertion in Python debug builds and is a better style anyway. --- src/pya/pya/pya.cc | 21 +++++++++++++++++++++ src/pya/pya/pya.h | 10 +++++++++- src/pya/pya/pyaCallables.cc | 3 +++ src/pya/pya/pyaModule.cc | 16 ++++++++++------ src/pya/pya/pyaModule.h | 6 ++++++ src/pymod/pymodHelper.h | 14 ++++++++++---- 6 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/pya/pya/pya.cc b/src/pya/pya/pya.cc index 5f177fa56e..0de6ea55ce 100644 --- a/src/pya/pya/pya.cc +++ b/src/pya/pya/pya.cc @@ -360,6 +360,10 @@ PythonInterpreter::PythonInterpreter (bool embedded) PythonInterpreter::~PythonInterpreter () { + for (auto m = m_modules.begin (); m != m_modules.end (); ++m) { + (*m)->cleanup (); + } + m_stdout_channel = PythonRef (); m_stderr_channel = PythonRef (); m_stdout = PythonPtr (); @@ -370,6 +374,23 @@ PythonInterpreter::~PythonInterpreter () if (m_embedded) { Py_Finalize (); } + + for (auto m = m_modules.begin (); m != m_modules.end (); ++m) { + delete *m; + } + m_modules.clear (); +} + +void +PythonInterpreter::register_module (pya::PythonModule *module) +{ + for (auto m = m_modules.begin (); m != m_modules.end (); ++m) { + if (*m == module) { + return; // already registered + } + } + + m_modules.push_back (module); } char * diff --git a/src/pya/pya/pya.h b/src/pya/pya/pya.h index ee8d35381b..76838f4f0f 100644 --- a/src/pya/pya/pya.h +++ b/src/pya/pya/pya.h @@ -105,6 +105,14 @@ class PYA_PUBLIC PythonInterpreter */ ~PythonInterpreter (); + /** + * @brief Registers a module + * + * The registered modules are cleaned up before the interpreter shuts down. The interpreter takes + * ownership of the module object. + */ + void register_module (pya::PythonModule *module); + /** * @brief Add the given path to the search path */ @@ -279,7 +287,7 @@ class PYA_PUBLIC PythonInterpreter std::map m_file_id_map; std::wstring mp_py3_app_name; bool m_embedded; - std::unique_ptr m_pya_module; + std::vector m_modules; }; } diff --git a/src/pya/pya/pyaCallables.cc b/src/pya/pya/pyaCallables.cc index 5dfc3e4f19..d12ea69054 100644 --- a/src/pya/pya/pyaCallables.cc +++ b/src/pya/pya/pyaCallables.cc @@ -51,6 +51,9 @@ pya_object_deallocate (PyObject *self) // we better work around it. ++self->ob_refcnt; + // Mute Python warnings in debug case + PyObject_GC_UnTrack (self); + PYAObjectBase *p = PYAObjectBase::from_pyobject (self); p->~PYAObjectBase (); Py_TYPE (self)->tp_free (self); diff --git a/src/pya/pya/pyaModule.cc b/src/pya/pya/pyaModule.cc index 30452d6694..62e6bacde2 100644 --- a/src/pya/pya/pyaModule.cc +++ b/src/pya/pya/pyaModule.cc @@ -71,12 +71,6 @@ PythonModule::PythonModule () PythonModule::~PythonModule () { - PYAObjectBase::clear_callbacks_cache (); - - // the Python objects were probably deleted by Python itself as it exited - - // don't try to delete them again. - mp_module.release (); - while (!m_methods_heap.empty ()) { delete m_methods_heap.back (); m_methods_heap.pop_back (); @@ -93,6 +87,16 @@ PythonModule::~PythonModule () } } +void +PythonModule::cleanup () +{ + // the Python objects are probably deleted by Python itself as it exits - + // don't try to delete them again in the destructor. + mp_module.release (); + + PYAObjectBase::clear_callbacks_cache (); +} + PyObject * PythonModule::module () { diff --git a/src/pya/pya/pyaModule.h b/src/pya/pya/pyaModule.h index 0adcc95861..b543ae02bf 100644 --- a/src/pya/pya/pyaModule.h +++ b/src/pya/pya/pyaModule.h @@ -65,6 +65,12 @@ class PYA_PUBLIC PythonModule */ ~PythonModule (); + /** + * @brief Clean up the module + * This method is called by the interpreter before Py_Finalize + */ + void cleanup (); + /** * @brief Initializes the module * This entry point is for external use where the module has not been created yet diff --git a/src/pymod/pymodHelper.h b/src/pymod/pymodHelper.h index 55bc56185a..6d116e234f 100644 --- a/src/pymod/pymodHelper.h +++ b/src/pymod/pymodHelper.h @@ -34,6 +34,7 @@ #include "pyaModule.h" #include "pyaUtils.h" +#include "pya.h" #include "gsi.h" #include "gsiExpression.h" @@ -41,7 +42,12 @@ static PyObject * module_init (const char *pymod_name, const char *mod_name, const char *mod_description) { - static pya::PythonModule module; + if (! pya::PythonInterpreter::instance ()) { + return 0; + } + + pya::PythonModule *module = new pya::PythonModule (); + pya::PythonInterpreter::instance ()->register_module (module); PYA_TRY @@ -50,10 +56,10 @@ module_init (const char *pymod_name, const char *mod_name, const char *mod_descr // required for the tiling processor for example gsi::initialize_expressions (); - module.init (pymod_name, mod_description); - module.make_classes (mod_name); + module->init (pymod_name, mod_description); + module->make_classes (mod_name); - return module.take_module (); + return module->take_module (); PYA_CATCH_ANYWHERE From 491048db4cb165418885bb88488f521af004d471 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sun, 8 Sep 2024 00:24:56 +0200 Subject: [PATCH 2/3] Fixed new Python module handling for standalone module case --- src/pya/pya/pya.cc | 4 +++- src/pya/pya/pyaModule.cc | 2 -- src/pya/pya/pyaObject.cc | 10 +++++++++- src/pya/pya/pyaObject.h | 2 +- src/pya/pya/pyaRefs.cc | 33 +++++++++++++++++++++++++-------- src/pya/pya/pyaRefs.h | 9 +++++++++ src/pymod/pymodHelper.h | 16 ++++++++-------- 7 files changed, 55 insertions(+), 21 deletions(-) diff --git a/src/pya/pya/pya.cc b/src/pya/pya/pya.cc index 0de6ea55ce..bc544e7f72 100644 --- a/src/pya/pya/pya.cc +++ b/src/pya/pya/pya.cc @@ -192,7 +192,7 @@ PythonInterpreter::PythonInterpreter (bool embedded) sp_interpreter = this; - // this monitor whether Python shuts down and deletes the interpreter's + // this monitors whether Python shuts down and deletes the interpreter's // instance. // NOTE: this assumes, the interpreter was created with new(!) Py_AtExit (&reset_interpreter); @@ -364,6 +364,8 @@ PythonInterpreter::~PythonInterpreter () (*m)->cleanup (); } + PYAObjectBase::clear_callbacks_cache (m_embedded); + m_stdout_channel = PythonRef (); m_stderr_channel = PythonRef (); m_stdout = PythonPtr (); diff --git a/src/pya/pya/pyaModule.cc b/src/pya/pya/pyaModule.cc index 62e6bacde2..86fa88fbac 100644 --- a/src/pya/pya/pyaModule.cc +++ b/src/pya/pya/pyaModule.cc @@ -93,8 +93,6 @@ PythonModule::cleanup () // the Python objects are probably deleted by Python itself as it exits - // don't try to delete them again in the destructor. mp_module.release (); - - PYAObjectBase::clear_callbacks_cache (); } PyObject * diff --git a/src/pya/pya/pyaObject.cc b/src/pya/pya/pyaObject.cc index 3887ed3a31..2bdc271830 100644 --- a/src/pya/pya/pyaObject.cc +++ b/src/pya/pya/pyaObject.cc @@ -514,8 +514,16 @@ PYAObjectBase::initialize_callbacks () } void -PYAObjectBase::clear_callbacks_cache () +PYAObjectBase::clear_callbacks_cache (bool embedded) { + // if not embedded, we cannot use the python API at this stage - do not try to + // reference count the objects there. + if (! embedded) { + for (auto c = s_callbacks_cache.begin (); c != s_callbacks_cache.end (); ++c) { + c->first.release_const (); + } + } + s_callbacks_cache.clear (); } diff --git a/src/pya/pya/pyaObject.h b/src/pya/pya/pyaObject.h index 96febf155c..86344a3286 100644 --- a/src/pya/pya/pyaObject.h +++ b/src/pya/pya/pyaObject.h @@ -200,7 +200,7 @@ class PYA_PUBLIC PYAObjectBase /** * @brief Clears the callbacks cache */ - static void clear_callbacks_cache (); + static void clear_callbacks_cache (bool embedded); private: friend class StatusChangedListener; diff --git a/src/pya/pya/pyaRefs.cc b/src/pya/pya/pyaRefs.cc index 49dc7049f0..420aa88eea 100644 --- a/src/pya/pya/pyaRefs.cc +++ b/src/pya/pya/pyaRefs.cc @@ -32,19 +32,19 @@ namespace pya // PythonRef implementation PythonRef::PythonRef () - : mp_obj (NULL) + : mp_obj (NULL), m_owns_pointer (true) { // .. nothing yet .. } PythonRef::PythonRef (const PythonPtr &ptr) - : mp_obj (ptr.get ()) + : mp_obj (ptr.get ()), m_owns_pointer (true) { Py_XINCREF (mp_obj); } PythonRef::PythonRef (PyObject *obj, bool new_ref) - : mp_obj (obj) + : mp_obj (obj), m_owns_pointer (true) { if (! new_ref) { Py_XINCREF (mp_obj); @@ -53,38 +53,49 @@ PythonRef::PythonRef (PyObject *obj, bool new_ref) PythonRef &PythonRef::operator= (PyObject *obj) { - Py_XDECREF (mp_obj); + if (m_owns_pointer) { + Py_XDECREF (mp_obj); + } mp_obj = obj; + m_owns_pointer = true; return *this; } PythonRef &PythonRef::operator= (const PythonPtr &ptr) { - Py_XDECREF (mp_obj); + if (m_owns_pointer) { + Py_XDECREF (mp_obj); + } mp_obj = ptr.get (); Py_XINCREF (mp_obj); + m_owns_pointer = true; return *this; } PythonRef &PythonRef::operator= (const PythonRef &other) { if (this != &other && mp_obj != other.mp_obj) { - Py_XDECREF (mp_obj); + if (m_owns_pointer) { + Py_XDECREF (mp_obj); + } mp_obj = other.mp_obj; + m_owns_pointer = true; Py_XINCREF (mp_obj); } return *this; } PythonRef::PythonRef (const PythonRef &other) - : mp_obj (other.mp_obj) + : mp_obj (other.mp_obj), m_owns_pointer (true) { Py_XINCREF (mp_obj); } PythonRef::~PythonRef () { - Py_XDECREF (mp_obj); + if (m_owns_pointer) { + Py_XDECREF (mp_obj); + } } PythonRef::operator bool () const @@ -109,6 +120,12 @@ PyObject *PythonRef::release () return o; } +PyObject *PythonRef::release_const () const +{ + m_owns_pointer = false; + return mp_obj; +} + // -------------------------------------------------------------------------- // PythonPtr implementation diff --git a/src/pya/pya/pyaRefs.h b/src/pya/pya/pyaRefs.h index 460aa7b372..c04673ae9b 100644 --- a/src/pya/pya/pyaRefs.h +++ b/src/pya/pya/pyaRefs.h @@ -116,6 +116,14 @@ class PYA_PUBLIC PythonRef */ PyObject *release (); + /** + * @brief Takes the pointer, but does not change the value + * This method will stop the reference from managing the object, but + * maintains the pointer. Do not access the pointer after this + * operation. + */ + PyObject *release_const () const; + /** * @brief Comparison operator */ @@ -134,6 +142,7 @@ class PYA_PUBLIC PythonRef private: PyObject *mp_obj; + mutable bool m_owns_pointer; }; /** diff --git a/src/pymod/pymodHelper.h b/src/pymod/pymodHelper.h index 6d116e234f..22557ed936 100644 --- a/src/pymod/pymodHelper.h +++ b/src/pymod/pymodHelper.h @@ -42,12 +42,7 @@ static PyObject * module_init (const char *pymod_name, const char *mod_name, const char *mod_description) { - if (! pya::PythonInterpreter::instance ()) { - return 0; - } - - pya::PythonModule *module = new pya::PythonModule (); - pya::PythonInterpreter::instance ()->register_module (module); + std::unique_ptr module (new pya::PythonModule ()); PYA_TRY @@ -59,10 +54,15 @@ module_init (const char *pymod_name, const char *mod_name, const char *mod_descr module->init (pymod_name, mod_description); module->make_classes (mod_name); - return module->take_module (); + PyObject *mod_object = module->take_module (); + + tl_assert (pya::PythonInterpreter::instance () != 0); + pya::PythonInterpreter::instance ()->register_module (module.release ()); + + return mod_object; PYA_CATCH_ANYWHERE - + return 0; } From c917831eb04c28003d8aa1a5b03d1a11927593a4 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sun, 15 Sep 2024 17:00:31 +0200 Subject: [PATCH 3/3] Fixed issue #1857 (crash when closing window with properties dialog open) --- src/layview/layview/layLayoutView_qt.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/layview/layview/layLayoutView_qt.cc b/src/layview/layview/layLayoutView_qt.cc index 86aeaf7743..7fb3d76e4e 100644 --- a/src/layview/layview/layLayoutView_qt.cc +++ b/src/layview/layview/layLayoutView_qt.cc @@ -650,6 +650,11 @@ void LayoutView::close() ms_current = 0; } + if (mp_properties_dialog) { + // must happen before "shutdown" (issue #1857) + delete mp_properties_dialog.data (); + } + // release all components and plugins before we delete the user interface shutdown (); @@ -687,10 +692,6 @@ void LayoutView::close() } mp_bookmarks_frame = 0; mp_bookmarks_view = 0; - - if (mp_properties_dialog) { - delete mp_properties_dialog.data (); - } } void