From 7d6a94db1d6131f70aa4d75ef8f7bc69ab0ac0fb Mon Sep 17 00:00:00 2001 From: Jim Carroll Date: Tue, 4 Dec 2012 09:13:45 -0500 Subject: [PATCH] This fixes the fact that python leaves objects hanging around. Also, it prevents the accidental callback from arriving on python objects where the interpreter has been long closed. closes #13624 --- xbmc/interfaces/legacy/CallbackHandler.cpp | 4 +- xbmc/interfaces/legacy/CallbackHandler.h | 4 +- xbmc/interfaces/python/CallbackHandler.cpp | 22 ++++- xbmc/interfaces/python/CallbackHandler.h | 4 +- xbmc/interfaces/python/LanguageHook.cpp | 86 ++++++++++++++----- xbmc/interfaces/python/LanguageHook.h | 24 +++++- .../interfaces/python/PythonSwig.cpp.template | 2 +- xbmc/interfaces/python/XBPyThread.cpp | 23 ++++- xbmc/interfaces/python/swig.cpp | 3 +- xbmc/interfaces/python/swig.h | 37 +++++++- 10 files changed, 170 insertions(+), 39 deletions(-) diff --git a/xbmc/interfaces/legacy/CallbackHandler.cpp b/xbmc/interfaces/legacy/CallbackHandler.cpp index 0cedb85c7c2e5..22ad95856354d 100644 --- a/xbmc/interfaces/legacy/CallbackHandler.cpp +++ b/xbmc/interfaces/legacy/CallbackHandler.cpp @@ -84,7 +84,7 @@ namespace XBMCAddon AddonClass::Ref p(*iter); // only call when we are in the right thread state - if(p->handler->isThreadStateOk()) + if(p->handler->isStateOk(p->cb->getObject())) { // remove it from the queue. No matter what we're done with // this. Even if it doesn't execute for some reason. @@ -140,7 +140,7 @@ namespace XBMCAddon { AddonClass::Ref p(*iter); - if(p->handler->shouldRemoveCallback(userData)) + if(p->handler->shouldRemoveCallback(p->cb->getObject(),userData)) { #ifdef ENABLE_TRACE_API CLog::Log(LOGDEBUG,"%sNEWADDON removing callback 0x%lx for PyThreadState 0x%lx from queue", _tg.getSpaces(),(long)(p->cb.get()) ,(long)userData); diff --git a/xbmc/interfaces/legacy/CallbackHandler.h b/xbmc/interfaces/legacy/CallbackHandler.h index 81793fdd04fc5..b4d6ff5c160a0 100644 --- a/xbmc/interfaces/legacy/CallbackHandler.h +++ b/xbmc/interfaces/legacy/CallbackHandler.h @@ -64,8 +64,8 @@ namespace XBMCAddon static void makePendingCalls(); static void clearPendingCalls(void* userData); - virtual bool isThreadStateOk() = 0; - virtual bool shouldRemoveCallback(void* userData) = 0; + virtual bool isStateOk(AddonClass* obj) = 0; + virtual bool shouldRemoveCallback(AddonClass* obj, void* userData) = 0; }; } diff --git a/xbmc/interfaces/python/CallbackHandler.cpp b/xbmc/interfaces/python/CallbackHandler.cpp index 21d401ea0ae79..979f0a61cf140 100644 --- a/xbmc/interfaces/python/CallbackHandler.cpp +++ b/xbmc/interfaces/python/CallbackHandler.cpp @@ -20,6 +20,7 @@ */ #include "CallbackHandler.h" +#include "LanguageHook.h" namespace XBMCAddon { @@ -42,10 +43,18 @@ namespace XBMCAddon * Now we are answering the question as to whether or not we are in the * PyThreadState that we were in when we started. */ - bool PythonCallbackHandler::isThreadStateOk() + bool PythonCallbackHandler::isStateOk(AddonClass* obj) { TRACE; - return objectThreadState == PyThreadState_Get(); + PyThreadState* state = PyThreadState_Get(); + if (objectThreadState == state) + { + // make sure the interpreter is still active. + AddonClass::Ref lh(XBMCAddon::Python::LanguageHook::getIfExists(state->interp)); + if (lh.isNotNull() && lh->hasRegisteredClass(obj)) + return true; + } + return false; } /** @@ -55,10 +64,15 @@ namespace XBMCAddon * TODO: This is a stupid way to get this information back to the handler. * there should be a more language neutral means. */ - bool PythonCallbackHandler::shouldRemoveCallback(void* threadState) + bool PythonCallbackHandler::shouldRemoveCallback(AddonClass* obj, void* threadState) { TRACE; - return threadState == objectThreadState; + if (threadState == objectThreadState) + return true; + + // we also want to remove the callback if the language hook no longer exists. + // this is a belt-and-suspenders cleanup mechanism + return ! XBMCAddon::Python::LanguageHook::oneHasRegisteredClass(obj); } } } diff --git a/xbmc/interfaces/python/CallbackHandler.h b/xbmc/interfaces/python/CallbackHandler.h index 3c219c8fd8b9b..e8f1836e49b28 100644 --- a/xbmc/interfaces/python/CallbackHandler.h +++ b/xbmc/interfaces/python/CallbackHandler.h @@ -44,8 +44,8 @@ namespace XBMCAddon * handling callbacks in the appropriate thread. */ PythonCallbackHandler(); - virtual bool isThreadStateOk(); - virtual bool shouldRemoveCallback(void* threadState); + virtual bool isStateOk(AddonClass* obj); + virtual bool shouldRemoveCallback(AddonClass* obj, void* threadState); }; } } diff --git a/xbmc/interfaces/python/LanguageHook.cpp b/xbmc/interfaces/python/LanguageHook.cpp index 788495fb70775..a986f0e280f7e 100644 --- a/xbmc/interfaces/python/LanguageHook.cpp +++ b/xbmc/interfaces/python/LanguageHook.cpp @@ -21,6 +21,7 @@ #include "LanguageHook.h" +#include "CallbackHandler.h" #include "XBPython.h" #include "interfaces/legacy/AddonUtils.h" @@ -33,15 +34,19 @@ namespace XBMCAddon { static AddonClass::Ref instance; - static CCriticalSection ccrit; - static bool isInited = false; - static xbmcutil::InitFlag flag(isInited); + static CCriticalSection hooksMutex; + std::map > LanguageHook::hooks; // vtab instantiation - LanguageHook::~LanguageHook() { } + LanguageHook::~LanguageHook() + { + TRACE; + XBMCAddon::LanguageHook::deallocating(); + } void LanguageHook::makePendingCalls() { + TRACE; PythonCallbackHandler::makePendingCalls(); } @@ -57,21 +62,37 @@ namespace XBMCAddon PyGILLock::acquireGil(); } - LanguageHook* LanguageHook::getInstance() + void LanguageHook::registerMe() { - if (!isInited) // in this case we're being called from a static initializer - { - if (instance.isNull()) - instance = new LanguageHook(); - } - else + TRACE; + CSingleLock lock(hooksMutex); + hooks[m_interp] = AddonClass::Ref(this); + } + + void LanguageHook::unregisterMe() + { + TRACE; + CSingleLock lock(hooksMutex); + hooks.erase(m_interp); + } + + AddonClass::Ref LanguageHook::getIfExists(PyInterpreterState* interp) + { + TRACE; + CSingleLock lock(hooksMutex); + std::map >::iterator iter = hooks.find(interp); + return iter == hooks.end() ? AddonClass::Ref(NULL) : AddonClass::Ref(iter->second); + } + + bool LanguageHook::oneHasRegisteredClass(AddonClass* obj) + { + for (std::map >::iterator iter = hooks.begin(); + iter != hooks.end(); iter++) { - CSingleLock lock (ccrit); - if (instance.isNull()) - instance = new LanguageHook(); + if ((iter->second)->hasRegisteredClass(obj)) + return true; } - - return instance.get(); + return false; } /** @@ -88,11 +109,13 @@ namespace XBMCAddon */ XBMCAddon::CallbackHandler* LanguageHook::getCallbackHandler() { + TRACE; return new PythonCallbackHandler(); } String LanguageHook::getAddonId() { + TRACE; const char* id = NULL; // Get a reference to the main module @@ -108,6 +131,7 @@ namespace XBMCAddon String LanguageHook::getAddonVersion() { + TRACE; // Get a reference to the main module // and global dictionary PyObject* main_module = PyImport_AddModule((char*)"__main__"); @@ -119,14 +143,36 @@ namespace XBMCAddon return version; } - void LanguageHook::registerPlayerCallback(IPlayerCallback* player) { g_pythonParser.RegisterPythonPlayerCallBack(player); } - void LanguageHook::unregisterPlayerCallback(IPlayerCallback* player) { g_pythonParser.UnregisterPythonPlayerCallBack(player); } - void LanguageHook::registerMonitorCallback(XBMCAddon::xbmc::Monitor* monitor) { g_pythonParser.RegisterPythonMonitorCallBack(monitor); } - void LanguageHook::unregisterMonitorCallback(XBMCAddon::xbmc::Monitor* monitor) { g_pythonParser.UnregisterPythonMonitorCallBack(monitor); } + void LanguageHook::registerPlayerCallback(IPlayerCallback* player) { TRACE; g_pythonParser.RegisterPythonPlayerCallBack(player); } + void LanguageHook::unregisterPlayerCallback(IPlayerCallback* player) { TRACE; g_pythonParser.UnregisterPythonPlayerCallBack(player); } + void LanguageHook::registerMonitorCallback(XBMCAddon::xbmc::Monitor* monitor) { TRACE; g_pythonParser.RegisterPythonMonitorCallBack(monitor); } + void LanguageHook::unregisterMonitorCallback(XBMCAddon::xbmc::Monitor* monitor) { TRACE; g_pythonParser.UnregisterPythonMonitorCallBack(monitor); } bool LanguageHook::waitForEvent(CEvent& hEvent, unsigned int milliseconds) { + TRACE; return g_pythonParser.WaitForEvent(hEvent,milliseconds); } + + void LanguageHook::registerAddonClass(AddonClass* obj) + { + TRACE; + Synchronize l(*this); + currentObjects.insert(obj); + } + + void LanguageHook::unregisterAddonClass(AddonClass* obj) + { + TRACE; + Synchronize l(*this); + currentObjects.erase(obj); + } + + bool LanguageHook::hasRegisteredClass(AddonClass* obj) + { + TRACE; + Synchronize l(*this); + return currentObjects.find(obj) != currentObjects.end(); + } } } diff --git a/xbmc/interfaces/python/LanguageHook.h b/xbmc/interfaces/python/LanguageHook.h index 34c1fcf063393..ef1c296c86082 100644 --- a/xbmc/interfaces/python/LanguageHook.h +++ b/xbmc/interfaces/python/LanguageHook.h @@ -28,10 +28,12 @@ #include #include "interfaces/legacy/LanguageHook.h" -#include "interfaces/python/CallbackHandler.h" #include "threads/ThreadLocal.h" #include "threads/Event.h" +#include +#include + namespace XBMCAddon { namespace Python @@ -45,10 +47,17 @@ namespace XBMCAddon */ class LanguageHook : public XBMCAddon::LanguageHook { - LanguageHook() : XBMCAddon::LanguageHook("Python::LanguageHook") { } + PyInterpreterState* m_interp; + CCriticalSection crit; + std::set currentObjects; + + static std::map > hooks; public: + inline LanguageHook(PyInterpreterState* interp) : + XBMCAddon::LanguageHook("Python::LanguageHook"), m_interp(interp) { } + virtual ~LanguageHook(); virtual void delayedCallOpen(); @@ -78,7 +87,16 @@ namespace XBMCAddon virtual void unregisterMonitorCallback(XBMCAddon::xbmc::Monitor* monitor); virtual bool waitForEvent(CEvent& hEvent, unsigned int milliseconds); - static LanguageHook* getInstance(); + static AddonClass::Ref getIfExists(PyInterpreterState* interp); + static bool oneHasRegisteredClass(AddonClass* obj); + + void registerAddonClass(AddonClass* obj); + void unregisterAddonClass(AddonClass* obj); + bool hasRegisteredClass(AddonClass* obj); + inline bool hasRegisteredClasses() { Synchronize l(*this); return currentObjects.size() > 0; } + + void unregisterMe(); + void registerMe(); }; } } diff --git a/xbmc/interfaces/python/PythonSwig.cpp.template b/xbmc/interfaces/python/PythonSwig.cpp.template index d3c106a8a90e3..8390d21fc3a7f 100644 --- a/xbmc/interfaces/python/PythonSwig.cpp.template +++ b/xbmc/interfaces/python/PythonSwig.cpp.template @@ -180,7 +180,7 @@ void doMethod(method, MethodType methodType) } // now do the method call itself if (!destructor) { - if (constructor || !clazz) { %> XBMCAddon::SetLanguageHookGuard slhg(XBMCAddon::Python::LanguageHook::getInstance());<% println() } + if (constructor || !clazz) { %> XBMCAddon::SetLanguageHookGuard slhg(XBMCAddon::Python::LanguageHook::getIfExists(PyThreadState_Get()->interp).get());<% println() } %> <% if (returns != "void") { %>apiResult = (${SwigTypeParser.SwigType_lstr(returns)})<% } if (clazz && !constructor) { diff --git a/xbmc/interfaces/python/XBPyThread.cpp b/xbmc/interfaces/python/XBPyThread.cpp index 1ffad6d49a557..ff96c97bda15b 100644 --- a/xbmc/interfaces/python/XBPyThread.cpp +++ b/xbmc/interfaces/python/XBPyThread.cpp @@ -158,6 +158,9 @@ void XBPyThread::Process() // swap in my thread state PyThreadState_Swap(state); + XBMCAddon::AddonClass::Ref languageHook(new XBMCAddon::Python::LanguageHook(state->interp)); + languageHook->registerMe(); + m_pExecuter->InitializeInterpreter(addon); CLog::Log(LOGDEBUG, "%s - The source file to load is %s", __FUNCTION__, m_source); @@ -369,8 +372,26 @@ void XBPyThread::Process() m_pExecuter->DeInitializeInterpreter(); Py_EndInterpreter(state); - PyThreadState_Swap(NULL); + // This is a total hack. Python doesn't necessarily release + // all of the objects associated with the interpreter when + // you end the interpreter. As a result there are objects + // managed by the windowing system that still receive events + // until python decides to clean them up. Python will eventually + // clean them up on the creation or ending of a subsequent + // interpreter. So we are going to keep creating and ending + // interpreters until we have no more python objects hanging + // around. + while (languageHook->hasRegisteredClasses()) + { + PyThreadState* tmpstate = Py_NewInterpreter(); + Py_EndInterpreter(tmpstate); + } + + // unregister the language hook prior to ending the interpreter + languageHook->unregisterMe(); + + PyThreadState_Swap(NULL); PyEval_ReleaseLock(); } diff --git a/xbmc/interfaces/python/swig.cpp b/xbmc/interfaces/python/swig.cpp index d8840692c67f8..57680e3345afe 100644 --- a/xbmc/interfaces/python/swig.cpp +++ b/xbmc/interfaces/python/swig.cpp @@ -202,7 +202,8 @@ namespace PythonBindings const char* methodNamespacePrefix, const char* methodNameForErrorString) throw (XBMCAddon::WrongTypeException) { if (pythonType == NULL || pythonType->magicNumber != XBMC_PYTHON_TYPE_MAGIC_NUMBER) - throw XBMCAddon::WrongTypeException("Non api type passed in place of the expected type \"%s.\"",expectedType); + throw XBMCAddon::WrongTypeException("Non api type passed to \"%s\" in place of the expected type \"%s.\"", + methodNameForErrorString, expectedType); if (!isParameterRightType(typeInfo->swigType,expectedType,methodNamespacePrefix)) { // maybe it's a child class diff --git a/xbmc/interfaces/python/swig.h b/xbmc/interfaces/python/swig.h index 22f899cbd68aa..69cd62a2a2970 100644 --- a/xbmc/interfaces/python/swig.h +++ b/xbmc/interfaces/python/swig.h @@ -28,6 +28,7 @@ #include "interfaces/legacy/AddonClass.h" #include "interfaces/legacy/Window.h" #include "threads/ThreadLocal.h" +#include "LanguageHook.h" namespace PythonBindings { @@ -104,14 +105,44 @@ namespace PythonBindings doretrieveApiInstance(((PyHolder*)pythonType),((PyHolder*)pythonType)->typeInfo, expectedType, methodNamespacePrefix, methodNameForErrorString); } - inline void prepareForReturn(XBMCAddon::AddonClass* c) { if(c) c->Acquire(); } + inline void prepareForReturn(XBMCAddon::AddonClass* c) + { + TRACE; + if(c) { + c->Acquire(); + PyThreadState* state = PyThreadState_Get(); + XBMCAddon::Python::LanguageHook::getIfExists(state->interp)->registerAddonClass(c); + } + } - inline void cleanForDealloc(XBMCAddon::AddonClass* c) { if(c) c->Release(); } + inline void cleanForDealloc(XBMCAddon::AddonClass* c) + { + TRACE; + if(c){ + PyThreadState* state = PyThreadState_Get(); + XBMCAddon::AddonClass::Ref lh = + XBMCAddon::Python::LanguageHook::getIfExists(state->interp); + if (lh.isNotNull()) lh->unregisterAddonClass(c); + c->Release(); + } + } /** * There is a Catch-22 in the destruction of a Window. This resolves that. */ - inline void cleanForDealloc(XBMCAddon::xbmcgui::Window* c) { if(c) { c->dispose(); c->Release(); } } + inline void cleanForDealloc(XBMCAddon::xbmcgui::Window* c) + { + TRACE; + if(c) + { + PyThreadState* state = PyThreadState_Get(); + XBMCAddon::AddonClass::Ref lh = + XBMCAddon::Python::LanguageHook::getIfExists(state->interp); + if (lh.isNotNull()) lh->unregisterAddonClass(c); + c->dispose(); + c->Release(); + } + } /** * This method allows for conversion of the native api Type to the Python type