Merge pull request #19062 from ksooo/pvr-fix-remove-channel-epg

[PVR] Fix deadlock while deleting EPG

5 files changed, 95 insertions, 51 deletions

diff --git a/xbmc/pvr/channels/PVRChannel.cpp b/xbmc/pvr/channels/PVRChannel.cpp
index 09a68fe784..7a8169baa6 100644
--- a/xbmc/pvr/channels/PVRChannel.cpp
+++ b/xbmc/pvr/channels/PVRChannel.cpp
@@ -118,10 +118,7 @@ bool CPVRChannel::QueueDelete()
 
   const std::shared_ptr<CPVREpg> epg = GetEPG();
   if (epg)
-  {
-    CServiceBroker::GetPVRManager().EpgContainer().QueueDeleteEpg(epg);
     ResetEPG();
-  }
 
   bReturn = database->QueueDeleteQuery(*this);
   return bReturn;
diff --git a/xbmc/pvr/channels/PVRChannel.h b/xbmc/pvr/channels/PVRChannel.h
index 0e59a9a416..ef4b6f4bbf 100644
--- a/xbmc/pvr/channels/PVRChannel.h
+++ b/xbmc/pvr/channels/PVRChannel.h
@@ -452,6 +452,16 @@ namespace PVR
      */
     void Notify(const PVREvent& event);
 
+    /*!
+     * @brief Lock the instance. No other thread gets access to this channel until Unlock was called.
+     */
+    void Lock() { m_critSection.lock(); }
+
+    /*!
+     * @brief Unlock the instance. Other threads may get access to this channel again.
+     */
+    void Unlock() { m_critSection.unlock(); }
+
     //@}
   private:
     CPVRChannel();
diff --git a/xbmc/pvr/channels/PVRChannelGroupInternal.cpp b/xbmc/pvr/channels/PVRChannelGroupInternal.cpp
index fe21c30975..74bc4c560d 100644
--- a/xbmc/pvr/channels/PVRChannelGroupInternal.cpp
+++ b/xbmc/pvr/channels/PVRChannelGroupInternal.cpp
@@ -17,7 +17,6 @@
 #include "pvr/addons/PVRClients.h"
 #include "pvr/channels/PVRChannel.h"
 #include "pvr/epg/EpgContainer.h"
-#include "pvr/epg/EpgDatabase.h"
 #include "utils/Variant.h"
 #include "utils/log.h"
 
@@ -282,46 +281,52 @@ bool CPVRChannelGroupInternal::AddAndUpdateChannels(const CPVRChannelGroup& chan
 std::vector<std::shared_ptr<CPVRChannel>> CPVRChannelGroupInternal::RemoveDeletedChannels(const CPVRChannelGroup& channels)
 {
   std::vector<std::shared_ptr<CPVRChannel>> removedChannels = CPVRChannelGroup::RemoveDeletedChannels(channels);
-
-  bool channelsDeleted = false;
-
-  const std::shared_ptr<CPVRDatabase> database = CServiceBroker::GetPVRManager().GetTVDatabase();
-  const std::shared_ptr<CPVREpgDatabase> epgDatabase =
-      CServiceBroker::GetPVRManager().EpgContainer().GetEpgDatabase();
-  if (!database || !epgDatabase)
-  {
-    CLog::LogF(LOGERROR, "No TV or EPG database");
-  }
-  else
+  if (!removedChannels.empty())
   {
-    // Note: We must lock the dbs the whole time, otherwise races may occur.
-    database->Lock();
-    epgDatabase->Lock();
+    bool channelsDeleted = false;
 
-    for (const auto& channel : removedChannels)
+    const std::shared_ptr<CPVRDatabase> database = CServiceBroker::GetPVRManager().GetTVDatabase();
+    if (!database)
     {
-      // since channel was not found in the internal group, it was deleted from the backend
-      channelsDeleted |= channel->QueueDelete();
+      CLog::LogF(LOGERROR, "No TV database");
+    }
+    else
+    {
+      std::vector<std::shared_ptr<CPVREpg>> epgsToRemove;
+
+      for (const auto& channel : removedChannels)
+      {
+        const auto epg = channel->GetEPG();
+        if (epg)
+          epgsToRemove.emplace_back(epg);
 
-      size_t queryCount = epgDatabase->GetDeleteQueriesCount();
-      if (queryCount > EPG_COMMIT_QUERY_COUNT_LIMIT)
-        epgDatabase->CommitDeleteQueries();
+        // Note: We need to obtain a lock for every channel instance before we can lock
+        //       the TV db. This order is important. Otherwise deadlocks may occur.
+        channel->Lock();
+      }
+
+      // Note: We must lock the db the whole time, otherwise races may occur.
+      database->Lock();
+      for (const auto& channel : removedChannels)
+      {
+        // since channel was not found in the internal group, it was deleted from the backend
+        channelsDeleted |= channel->QueueDelete();
+        channel->Unlock();
+
+        size_t queryCount = database->GetDeleteQueriesCount();
+        if (queryCount > CHANNEL_COMMIT_QUERY_COUNT_LIMIT)
+          database->CommitDeleteQueries();
+      }
 
-      queryCount = database->GetDeleteQueriesCount();
-      if (queryCount > CHANNEL_COMMIT_QUERY_COUNT_LIMIT)
+      if (channelsDeleted)
         database->CommitDeleteQueries();
-    }
 
-    if (channelsDeleted)
-    {
-      epgDatabase->CommitDeleteQueries();
-      database->CommitDeleteQueries();
-    }
+      database->Unlock();
 
-    epgDatabase->Unlock();
-    database->Unlock();
+      // delete the EPG data for the removed channels
+      CServiceBroker::GetPVRManager().EpgContainer().QueueDeleteEpgs(epgsToRemove);
+    }
   }
-
   return removedChannels;
 }
 
diff --git a/xbmc/pvr/epg/EpgContainer.cpp b/xbmc/pvr/epg/EpgContainer.cpp
index 08e68e4add..867c6db0e4 100644
--- a/xbmc/pvr/epg/EpgContainer.cpp
+++ b/xbmc/pvr/epg/EpgContainer.cpp
@@ -268,7 +268,7 @@ bool CPVREpgContainer::PersistAll(unsigned int iMaxTimeslice) const
       if (epg.second && epg.second->NeedsSave())
       {
         // Note: We need to obtain a lock for every epg instance before we can lock
-        //       the epg db. This order is important. Otherwise deadlocks may occure.
+        //       the epg db. This order is important. Otherwise deadlocks may occur.
         epg.second->Lock();
         changedEpgs.emplace_back(epg.second);
       }
@@ -279,7 +279,7 @@ bool CPVREpgContainer::PersistAll(unsigned int iMaxTimeslice) const
 
   if (!changedEpgs.empty())
   {
-    // Note: We must lock the db the whole time, otherwise races may occure.
+    // Note: We must lock the db the whole time, otherwise races may occur.
     database->Lock();
 
     XbmcThreads::EndTime processTimeslice(iMaxTimeslice);
@@ -612,6 +612,41 @@ bool CPVREpgContainer::RemoveOldEntries()
   return true;
 }
 
+bool CPVREpgContainer::QueueDeleteEpgs(const std::vector<std::shared_ptr<CPVREpg>>& epgs)
+{
+  if (epgs.empty())
+    return true;
+
+  const std::shared_ptr<CPVREpgDatabase> database = GetEpgDatabase();
+  if (!database)
+  {
+    CLog::LogF(LOGERROR, "No EPG database");
+    return false;
+  }
+
+  for (const auto& epg : epgs)
+  {
+    // Note: We need to obtain a lock for every epg instance before we can lock
+    //       the epg db. This order is important. Otherwise deadlocks may occur.
+    epg->Lock();
+  }
+
+  database->Lock();
+  for (const auto& epg : epgs)
+  {
+    QueueDeleteEpg(epg);
+    epg->Unlock();
+
+    size_t queryCount = database->GetDeleteQueriesCount();
+    if (queryCount > EPG_COMMIT_QUERY_COUNT_LIMIT)
+      database->CommitDeleteQueries();
+  }
+  database->CommitDeleteQueries();
+  database->Unlock();
+
+  return true;
+}
+
 bool CPVREpgContainer::QueueDeleteEpg(const std::shared_ptr<CPVREpg>& epg)
 {
   if (!epg || epg->EpgID() < 0)
@@ -747,17 +782,7 @@ bool CPVREpgContainer::UpdateEPG(bool bOnlyPending /* = false */)
   if (bShowProgress && !bOnlyPending)
     progressHandler->DestroyProgress();
 
-  database->Lock();
-  for (const auto& epg : invalidTables)
-  {
-    QueueDeleteEpg(epg);
-
-    size_t queryCount = database->GetDeleteQueriesCount();
-    if (queryCount > EPG_COMMIT_QUERY_COUNT_LIMIT)
-      database->CommitDeleteQueries();
-  }
-  database->CommitDeleteQueries();
-  database->Unlock();
+  QueueDeleteEpgs(invalidTables);
 
   if (bInterrupted)
   {
diff --git a/xbmc/pvr/epg/EpgContainer.h b/xbmc/pvr/epg/EpgContainer.h
index 19ab97003e..0847c8a8de 100644
--- a/xbmc/pvr/epg/EpgContainer.h
+++ b/xbmc/pvr/epg/EpgContainer.h
@@ -91,11 +91,11 @@ namespace PVR
     bool IsStarted() const;
 
     /*!
-     * @brief Queue the deletion of an EPG table from this container.
-     * @param epg The table to delete.
+     * @brief Queue the deletion of the given EPG tables from this container.
+     * @param epg The tables to delete.
      * @return True on success, false otherwise.
      */
-    bool QueueDeleteEpg(const std::shared_ptr<CPVREpg>& epg);
+    bool QueueDeleteEpgs(const std::vector<std::shared_ptr<CPVREpg>>& epgs);
 
     /*!
      * @brief CEventStream callback for PVR events.
@@ -280,6 +280,13 @@ namespace PVR
      */
     void InsertFromDB(const std::shared_ptr<CPVREpg>& newEpg);
 
+    /*!
+     * @brief Queue the deletion of an EPG table from this container.
+     * @param epg The table to delete.
+     * @return True on success, false otherwise.
+     */
+    bool QueueDeleteEpg(const std::shared_ptr<CPVREpg>& epg);
+
     std::shared_ptr<CPVREpgDatabase> m_database; /*!< the EPG database */
 
     bool m_bIsUpdating = false; /*!< true while an update is running */