Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge pull request #2191 from jimfcarroll/fix-14048

Fix #14048 - Change the locking in XBPython/XBPyThread
  • Loading branch information...
commit 94c518f2fdeb1813d0e53dfb95f98fd999ecd60e 2 parents c2a21de + 6f87d6b
Jim Carroll authored
View
18 xbmc/interfaces/python/XBPyThread.cpp
@@ -261,7 +261,7 @@ void XBPyThread::Process()
// we need to check if we was asked to abort before we had inited
bool stopping = false;
- { CSingleLock lock(m_pExecuter->m_critSection);
+ { CSingleLock lock(m_critSec);
m_threadState = state;
stopping = m_stopping;
}
@@ -389,13 +389,13 @@ void XBPyThread::Process()
PyEval_ReleaseLock();
//set stopped event - this allows ::stop to run and kill remaining threads
- //this event has to be fired without holding m_pExecuter->m_critSection
- //before
+ //this event has to be fired without holding m_critSec
+ //
//Also the GIL (PyEval_AcquireLock) must not be held
//if not obeyed there is still no deadlock because ::stop waits with timeout (smart one!)
stoppedEvent.Set();
- { CSingleLock lock(m_pExecuter->m_critSection);
+ { CSingleLock lock(m_critSec);
m_threadState = NULL;
}
@@ -440,7 +440,7 @@ void XBPyThread::OnException()
PyThreadState_Swap(NULL);
PyEval_ReleaseLock();
- CSingleLock lock(m_pExecuter->m_critSection);
+ CSingleLock lock(m_critSec);
m_threadState = NULL;
CLog::Log(LOGERROR,"%s, abnormally terminating python thread", __FUNCTION__);
m_pExecuter->setDone(m_id);
@@ -452,7 +452,7 @@ bool XBPyThread::isStopping() {
void XBPyThread::stop()
{
- CSingleLock lock(m_pExecuter->m_critSection);
+ CSingleLock lock(m_critSec);
if(m_stopping)
return;
@@ -499,12 +499,12 @@ void XBPyThread::stop()
//everything which didn't exit by now gets killed
{
- // grabbing the PyLock while holding the XBPython m_critSection is asking for a deadlock
- CSingleExit ex2(m_pExecuter->m_critSection);
+ // grabbing the PyLock while holding the m_critSec is asking for a deadlock
+ CSingleExit ex2(m_critSec);
PyEval_AcquireLock();
}
- // since we released the XBPython m_critSection it's possible that the state is cleaned up
+ // Since we released the m_critSec it's possible that the state is cleaned up
// so we need to recheck for m_threadState == NULL
if (m_threadState)
{
View
2  xbmc/interfaces/python/XBPyThread.h
@@ -24,6 +24,7 @@
#include "threads/Thread.h"
#include "threads/Event.h"
+#include "threads/CriticalSection.h"
#include "addons/IAddon.h"
class XBPython;
@@ -42,6 +43,7 @@ class XBPyThread : public CThread
void setAddon(ADDON::AddonPtr _addon) { addon = _addon; }
protected:
+ CCriticalSection m_critSec;
XBPython *m_pExecuter;
CEvent stoppedEvent;
void *m_threadState;
View
271 xbmc/interfaces/python/XBPython.cpp
@@ -26,6 +26,8 @@
// python.h should always be included first before any other includes
#include <Python.h>
+#include <algorithm>
+
#include "system.h"
#include "cores/DllLoader/DllLoaderContainer.h"
#include "GUIPassword.h"
@@ -79,20 +81,24 @@ XBPython::~XBPython()
CAnnouncementManager::RemoveAnnouncer(this);
}
+#define LOCK_AND_COPY(type, dest, src) \
+ if (!m_bInitialized) return; \
+ CSingleLock lock(src); \
+ src.hadSomethingRemoved = false; \
+ type dest; \
+ dest = src
+
+#define CHECK_FOR_ENTRY(l,v) \
+ (l.hadSomethingRemoved ? (std::find(l.begin(),l.end(),v) != l.end()) : true)
+
// message all registered callbacks that xbmc stopped playing
void XBPython::OnPlayBackEnded()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackEnded();
- it++;
- }
- }
}
void XBPython::Announce(AnnouncementFlag flag, const char *sender, const char *message, const CVariant &data)
@@ -124,145 +130,101 @@ void XBPython::Announce(AnnouncementFlag flag, const char *sender, const char *m
void XBPython::OnPlayBackStarted()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackStarted();
- it++;
- }
- }
}
// message all registered callbacks that we paused playing
void XBPython::OnPlayBackPaused()
{
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ TRACE;
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackPaused();
- it++;
- }
- }
}
// message all registered callbacks that we resumed playing
void XBPython::OnPlayBackResumed()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackResumed();
- it++;
- }
- }
}
// message all registered callbacks that user stopped playing
void XBPython::OnPlayBackStopped()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackStopped();
- it++;
- }
- }
}
// message all registered callbacks that playback speed changed (FF/RW)
void XBPython::OnPlayBackSpeedChanged(int iSpeed)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackSpeedChanged(iSpeed);
- it++;
- }
- }
}
// message all registered callbacks that player is seeking
void XBPython::OnPlayBackSeek(int iTime, int seekOffset)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackSeek(iTime, seekOffset);
- it++;
- }
- }
}
// message all registered callbacks that player chapter seeked
void XBPython::OnPlayBackSeekChapter(int iChapter)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnPlayBackSeekChapter(iChapter);
- it++;
- }
- }
}
// message all registered callbacks that next item has been queued
void XBPython::OnQueueNextItem()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
- while (it != m_vecPlayerCallbackList.end())
- {
+ LOCK_AND_COPY(std::vector<PVOID>,tmp,m_vecPlayerCallbackList);
+ for (PlayerCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecPlayerCallbackList,(*it)))
((IPlayerCallback*)(*it))->OnQueueNextItem();
- it++;
- }
- }
}
void XBPython::RegisterPythonPlayerCallBack(IPlayerCallback* pCallback)
{
TRACE;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPlayerCallbackList);
m_vecPlayerCallbackList.push_back(pCallback);
}
void XBPython::UnregisterPythonPlayerCallBack(IPlayerCallback* pCallback)
{
TRACE;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPlayerCallbackList);
PlayerCallbackList::iterator it = m_vecPlayerCallbackList.begin();
while (it != m_vecPlayerCallbackList.end())
{
if (*it == pCallback)
+ {
it = m_vecPlayerCallbackList.erase(it);
+ m_vecPlayerCallbackList.hadSomethingRemoved = true;
+ }
else
it++;
}
@@ -271,19 +233,22 @@ void XBPython::UnregisterPythonPlayerCallBack(IPlayerCallback* pCallback)
void XBPython::RegisterPythonMonitorCallBack(XBMCAddon::xbmc::Monitor* pCallback)
{
TRACE;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecMonitorCallbackList);
m_vecMonitorCallbackList.push_back(pCallback);
}
void XBPython::UnregisterPythonMonitorCallBack(XBMCAddon::xbmc::Monitor* pCallback)
{
TRACE;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecMonitorCallbackList);
MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
while (it != m_vecMonitorCallbackList.end())
{
if (*it == pCallback)
+ {
it = m_vecMonitorCallbackList.erase(it);
+ m_vecMonitorCallbackList.hadSomethingRemoved = true;
+ }
else
it++;
}
@@ -292,98 +257,60 @@ void XBPython::UnregisterPythonMonitorCallBack(XBMCAddon::xbmc::Monitor* pCallba
void XBPython::OnSettingsChanged(const CStdString &ID)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
- while (it != m_vecMonitorCallbackList.end())
- {
- if (((XBMCAddon::xbmc::Monitor*)(*it))->GetId() == ID)
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnSettingsChanged();
- it++;
- }
- }
+ LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+ for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecMonitorCallbackList,(*it)) && ((*it)->GetId() == ID))
+ (*it)->OnSettingsChanged();
}
void XBPython::OnScreensaverActivated()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
- while (it != m_vecMonitorCallbackList.end())
- {
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnScreensaverActivated();
- it++;
- }
- }
+ LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+ for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecMonitorCallbackList,(*it)))
+ (*it)->OnScreensaverActivated();
}
void XBPython::OnScreensaverDeactivated()
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
- while (it != m_vecMonitorCallbackList.end())
- {
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnScreensaverDeactivated();
- it++;
- }
- }
+ LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+ for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecMonitorCallbackList,(*it)))
+ (*it)->OnScreensaverDeactivated();
}
void XBPython::OnDatabaseUpdated(const std::string &database)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
- while (it != m_vecMonitorCallbackList.end())
- {
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnDatabaseUpdated(database);
- it++;
- }
- }
+ LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+ for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecMonitorCallbackList,(*it)))
+ (*it)->OnDatabaseUpdated(database);
}
void XBPython::OnDatabaseScanStarted(const std::string &database)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
- while (it != m_vecMonitorCallbackList.end())
- {
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnDatabaseScanStarted(database);
- it++;
- }
- }
+ LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+ for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
+ if (CHECK_FOR_ENTRY(m_vecMonitorCallbackList,(*it)))
+ (*it)->OnDatabaseScanStarted(database);
}
void XBPython::OnAbortRequested(const CStdString &ID)
{
TRACE;
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
+ LOCK_AND_COPY(std::vector<XBMCAddon::xbmc::Monitor*>,tmp,m_vecMonitorCallbackList);
+ for (MonitorCallbackList::iterator it = tmp.begin(); (it != tmp.end()); it++)
{
- MonitorCallbackList::iterator it = m_vecMonitorCallbackList.begin();
- while (it != m_vecMonitorCallbackList.end())
+ if (CHECK_FOR_ENTRY(m_vecMonitorCallbackList,(*it)))
{
if (ID.IsEmpty())
- {
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnAbortRequested();
- }
- else
- {
- if (((XBMCAddon::xbmc::Monitor*)(*it))->GetId() == ID)
- ((XBMCAddon::xbmc::Monitor*)(*it))->OnAbortRequested();
- }
- it++;
+ (*it)->OnAbortRequested();
+ else if ((*it)->GetId() == ID)
+ (*it)->OnAbortRequested();
}
}
}
@@ -633,6 +560,8 @@ void XBPython::FinalizeScript()
m_endtime = XbmcThreads::SystemClockMillis();
}
+
+// Always called with the lock held on m_critSection
void XBPython::Finalize()
{
TRACE;
@@ -673,19 +602,14 @@ void XBPython::Finalize()
void XBPython::FreeResources()
{
- CSingleLock lock(m_critSection);
- if (m_bInitialized)
- {
- // with the m_critSection held, we should copy the PyList so that
- // we can operate on the values once we release it.
- PyList tmpvec = m_vecPyList;
- m_vecPyList.clear();
+ LOCK_AND_COPY(std::vector<PyElem>,tmpvec,m_vecPyList);
+ m_vecPyList.clear();
+ m_vecPyList.hadSomethingRemoved = true;
- lock.Leave(); //unlock here because the python thread might lock when it exits
+ lock.Leave(); //unlock here because the python thread might lock when it exits
- // cleanup threads that are still running
- tmpvec.clear(); // boost releases the XBPyThreads which, if deleted, calls FinalizeScript
- }
+ // cleanup threads that are still running
+ tmpvec.clear(); // boost releases the XBPyThreads which, if deleted, calls FinalizeScript
}
void XBPython::Process()
@@ -703,7 +627,7 @@ void XBPython::Process()
CLog::Log(LOGDEBUG, "%s - no profile autoexec.py (%s) found, skipping", __FUNCTION__, strAutoExecPy.c_str());
}
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
if (m_bInitialized)
{
@@ -714,6 +638,7 @@ void XBPython::Process()
{
tmpvec.push_back(*it);
it = m_vecPyList.erase(it);
+ m_vecPyList.hadSomethingRemoved = true;
}
else
it++;
@@ -723,8 +648,11 @@ void XBPython::Process()
//delete scripts which are done
tmpvec.clear(); // boost releases the XBPyThreads which, if deleted, calls FinalizeScript
+ CSingleLock l2(m_critSection);
if(m_iDllScriptCounter == 0 && (XbmcThreads::SystemClockMillis() - m_endtime) > 10000 )
+ {
Finalize();
+ }
}
}
@@ -766,7 +694,7 @@ int XBPython::evalFile(const CStdString &src, const std::vector<CStdString> &arg
if (g_settings.GetCurrentProfile().programsLocked() && !g_passwordManager.IsMasterLockUnlocked(true))
return -1;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
Initialize();
if (!m_bInitialized) return -1;
@@ -789,7 +717,7 @@ int XBPython::evalFile(const CStdString &src, const std::vector<CStdString> &arg
void XBPython::setDone(int id)
{
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
PyList::iterator it = m_vecPyList.begin();
while (it != m_vecPyList.end())
{
@@ -808,7 +736,7 @@ void XBPython::setDone(int id)
void XBPython::stopScript(int id)
{
CSingleExit ex(g_graphicsContext);
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
PyList::iterator it = m_vecPyList.begin();
while (it != m_vecPyList.end())
{
@@ -829,7 +757,7 @@ void* XBPython::getMainThreadState()
int XBPython::ScriptsSize()
{
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
return m_vecPyList.size();
}
@@ -837,7 +765,7 @@ const char* XBPython::getFileName(int scriptId)
{
const char* cFileName = NULL;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
PyList::iterator it = m_vecPyList.begin();
while (it != m_vecPyList.end())
{
@@ -853,7 +781,7 @@ int XBPython::getScriptId(const CStdString &strFile)
{
int iId = -1;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
PyList::iterator it = m_vecPyList.begin();
while (it != m_vecPyList.end())
@@ -868,7 +796,7 @@ int XBPython::getScriptId(const CStdString &strFile)
bool XBPython::isRunning(int scriptId)
{
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
for(PyList::iterator it = m_vecPyList.begin(); it != m_vecPyList.end(); it++)
{
@@ -887,7 +815,7 @@ bool XBPython::isStopping(int scriptId)
{
bool bStopping = false;
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
PyList::iterator it = m_vecPyList.begin();
while (it != m_vecPyList.end())
{
@@ -901,7 +829,7 @@ bool XBPython::isStopping(int scriptId)
int XBPython::GetPythonScriptId(int scriptPosition)
{
- CSingleLock lock(m_critSection);
+ CSingleLock lock(m_vecPyList);
return (int)m_vecPyList[scriptPosition].id;
}
@@ -946,6 +874,9 @@ int XBPython::evalString(const CStdString &src, const std::vector<CStdString> &a
inf.strFile = "<string>";
inf.pyThread = pyThread;
+ lock.Leave();
+ CSingleLock l2(m_vecPyList);
+
m_vecPyList.push_back(inf);
return m_nextid;
View
11 xbmc/interfaces/python/XBPython.h
@@ -47,9 +47,12 @@ namespace XBMCAddon
}
}
-typedef std::vector<PyElem> PyList;
-typedef std::vector<PVOID> PlayerCallbackList;
-typedef std::vector<XBMCAddon::xbmc::Monitor*> MonitorCallbackList;
+template <class T> struct LockableType : public T, public CCriticalSection
+{ bool hadSomethingRemoved; };
+
+typedef LockableType<std::vector<PVOID> > PlayerCallbackList;
+typedef LockableType<std::vector<XBMCAddon::xbmc::Monitor*> > MonitorCallbackList;
+typedef LockableType<std::vector<PyElem> > PyList;
typedef std::vector<LibraryLoader*> PythonExtensionLibraries;
class XBPython :
@@ -128,8 +131,8 @@ class XBPython :
void* getMainThreadState();
bool m_bLogin;
- CCriticalSection m_critSection;
private:
+ CCriticalSection m_critSection;
bool FileExist(const char* strFile);
int m_nextid;
Please sign in to comment.
Something went wrong with that request. Please try again.