From 958822569609cbcf4876cc577b24addfc85d0fe6 Mon Sep 17 00:00:00 2001 From: Thomas Amland Date: Sun, 31 Jul 2016 19:13:52 +0200 Subject: [listprovider] fix race conditions when changing update state this ensures the job callback never overwrites invalididation of the content done by another thread --- xbmc/listproviders/DirectoryProvider.cpp | 41 +++++++++++++------------------- xbmc/listproviders/DirectoryProvider.h | 3 +-- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/xbmc/listproviders/DirectoryProvider.cpp b/xbmc/listproviders/DirectoryProvider.cpp index d01d6fa07d..b50833cd2e 100644 --- a/xbmc/listproviders/DirectoryProvider.cpp +++ b/xbmc/listproviders/DirectoryProvider.cpp @@ -192,26 +192,26 @@ bool CDirectoryProvider::Update(bool forceRefresh) // we never need to force refresh here bool changed = false; bool fireJob = false; - { - CSingleLock lock(m_section); - if (m_updateState == DONE) - changed = true; - else if (m_updateState == PENDING) - fireJob = true; - m_updateState = OK; - } // update the URL & limit and fire off a new job if needed fireJob |= UpdateURL(); fireJob |= UpdateSort(); fireJob |= UpdateLimit(); + + CSingleLock lock(m_section); + if (m_updateState == INVALIDATED) + fireJob = true; + else if (m_updateState == DONE) + changed = true; + + m_updateState = OK; + if (fireJob) { - { - CSingleLock lock(m_section); - CLog::Log(LOGDEBUG, "CDirectoryProvider[%s]: refreshing..", m_currentUrl.c_str()); - } - FireJob(); + CLog::Log(LOGDEBUG, "CDirectoryProvider[%s]: refreshing..", m_currentUrl.c_str()); + if (m_jobID) + CJobManager::GetInstance().CancelJob(m_jobID); + m_jobID = CJobManager::GetInstance().AddJob(new CDirectoryJob(m_currentUrl, m_currentSort, m_currentLimit, m_parentID), this); } for (std::vector::iterator i = m_items.begin(); i != m_items.end(); ++i) @@ -245,7 +245,7 @@ void CDirectoryProvider::Announce(AnnouncementFlag flag, const char *sender, con strcmp(message, "OnCleanFinished") == 0 || strcmp(message, "OnUpdate") == 0 || strcmp(message, "OnRemove") == 0) - m_updateState = PENDING; + m_updateState = INVALIDATED; } } @@ -269,7 +269,7 @@ void CDirectoryProvider::OnEvent(const ADDON::AddonEvent& event) typeid(event) == typeid(ADDON::AddonEvents::Disabled) || typeid(event) == typeid(ADDON::AddonEvents::InstalledChanged) || typeid(event) == typeid(ADDON::AddonEvents::MetadataChanged)) - m_updateState = PENDING; + m_updateState = INVALIDATED; } } @@ -303,7 +303,8 @@ void CDirectoryProvider::OnJobComplete(unsigned int jobID, bool success, CJob *j m_items = ((CDirectoryJob*)job)->GetItems(); m_currentTarget = ((CDirectoryJob*)job)->GetTarget(); ((CDirectoryJob*)job)->GetItemTypes(m_itemTypes); - m_updateState = DONE; + if (m_updateState == OK) + m_updateState = DONE; } m_jobID = 0; } @@ -370,14 +371,6 @@ bool CDirectoryProvider::IsUpdating() const return m_jobID || (m_updateState == DONE); } -void CDirectoryProvider::FireJob() -{ - CSingleLock lock(m_section); - if (m_jobID) - CJobManager::GetInstance().CancelJob(m_jobID); - m_jobID = CJobManager::GetInstance().AddJob(new CDirectoryJob(m_currentUrl, m_currentSort, m_currentLimit, m_parentID), this); -} - void CDirectoryProvider::RegisterListProvider() { CSingleLock lock(m_section); diff --git a/xbmc/listproviders/DirectoryProvider.h b/xbmc/listproviders/DirectoryProvider.h index 1b0a7e367c..d5de8c99ec 100644 --- a/xbmc/listproviders/DirectoryProvider.h +++ b/xbmc/listproviders/DirectoryProvider.h @@ -49,7 +49,7 @@ public: typedef enum { OK, - PENDING, + INVALIDATED, DONE } UpdateState; @@ -84,7 +84,6 @@ private: std::vector m_itemTypes; CCriticalSection m_section; - void FireJob(); void RegisterListProvider(); bool UpdateURL(); bool UpdateLimit(); -- cgit v1.2.3 From 637b0af864eb5b5cb6d668056461675b93af4a3b Mon Sep 17 00:00:00 2001 From: Thomas Amland Date: Sun, 31 Jul 2016 19:16:12 +0200 Subject: [listprovider] fix IsUpdating must include INVALIDATED such that Update can be called when invalidated by another thread --- xbmc/listproviders/DirectoryProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xbmc/listproviders/DirectoryProvider.cpp b/xbmc/listproviders/DirectoryProvider.cpp index b50833cd2e..241a1cb18d 100644 --- a/xbmc/listproviders/DirectoryProvider.cpp +++ b/xbmc/listproviders/DirectoryProvider.cpp @@ -368,7 +368,7 @@ bool CDirectoryProvider::OnContextMenu(const CGUIListItemPtr& item) bool CDirectoryProvider::IsUpdating() const { CSingleLock lock(m_section); - return m_jobID || (m_updateState == DONE); + return m_jobID || m_updateState == DONE || m_updateState == INVALIDATED; } void CDirectoryProvider::RegisterListProvider() -- cgit v1.2.3 From a0f72af516b6720a5cfd213396fa78b738ce3fa2 Mon Sep 17 00:00:00 2001 From: Thomas Amland Date: Sun, 31 Jul 2016 19:25:14 +0200 Subject: [listprovider] skip some checks when possible --- xbmc/listproviders/DirectoryProvider.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/xbmc/listproviders/DirectoryProvider.cpp b/xbmc/listproviders/DirectoryProvider.cpp index 241a1cb18d..17657413eb 100644 --- a/xbmc/listproviders/DirectoryProvider.cpp +++ b/xbmc/listproviders/DirectoryProvider.cpp @@ -214,8 +214,11 @@ bool CDirectoryProvider::Update(bool forceRefresh) m_jobID = CJobManager::GetInstance().AddJob(new CDirectoryJob(m_currentUrl, m_currentSort, m_currentLimit, m_parentID), this); } - for (std::vector::iterator i = m_items.begin(); i != m_items.end(); ++i) - changed |= (*i)->UpdateVisibility(m_parentID); + if (!changed) + { + for (std::vector::iterator i = m_items.begin(); i != m_items.end(); ++i) + changed |= (*i)->UpdateVisibility(m_parentID); + } return changed; //! @todo Also returned changed if properties are changed (if so, need to update scroll to letter). } -- cgit v1.2.3