fixed errors (deadlock, segfaults)

source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp

@@ -855,7 +855,7 @@ namespace armarx::plugins
             s.executable = false;
             s.native = true;
 
-            std::shared_lock providersLock_shared(fluxioDC.providersMutex);
+            std::unique_lock providersLock(fluxioDC.providersMutex);
             const auto& providersEntry =
                 fluxioDC.providers.find(boost::uuids::to_string(providerId));
             if (providersEntry != fluxioDC.providers.end())
@@ -864,19 +864,18 @@ namespace armarx::plugins
             }
             else
             {
-                providersLock_shared.unlock();
-                std::unique_lock providersLock_unique(fluxioDC.providersMutex);
                 const auto p = addFluxioProvider(skillId.providerId->providerName);
                 if (p == nullptr)
                 {
                     ARMARX_WARNING << "Failed to add provider with name '"
-                                   << skillId.providerId->providerName << "'.";
-                    providersLock_unique.unlock();
+                                   << skillId.providerId->providerName << "'. Skill '" << s.name
+                                   << " (" << s.id << ")' will not be added.";
+                    providersLock.unlock();
                     continue;
                 }
                 s.skillProviderPtr = p;
-                providersLock_unique.unlock();
             }
+            providersLock.unlock();
 
             // add default control flow parameters
             skills::FluxioParameter start;
@@ -965,15 +964,15 @@ namespace armarx::plugins
     {
         std::shared_lock l(fluxioDC.skillsMutex);
         const auto& skillsEntry = fluxioDC.skills.find(id);
-        if (skillsEntry != fluxioDC.skills.end())
+        if (skillsEntry == fluxioDC.skills.end())
         {
+            ARMARX_WARNING << "Skill with id '" << id << "' not found.";
             l.unlock();
-            return std::experimental::make_observer(&skillsEntry->second);
+            return nullptr;
         }
 
-        ARMARX_WARNING << "Skill with id '" << id << "' not found.";
         l.unlock();
-        return nullptr;
+        return std::experimental::make_observer(&skillsEntry->second);
     }
 
     std::optional<std::vector<std::experimental::observer_ptr<const skills::FluxioSkill>>>
@@ -1017,6 +1016,12 @@ namespace armarx::plugins
 
             for (const auto& [nodeId, node] : s.nodes)
             {
+                if (node == nullptr)
+                {
+                    ARMARX_WARNING << "Unexpected nullptr!";
+                    continue;
+                }
+
                 if (node->nodeType == skills::FluxioNodeType::SUBSKILL)
                 {
                     // cast to the right node type
@@ -1062,8 +1067,7 @@ namespace armarx::plugins
             ARMARX_WARNING << "Updating affected skills.";
             for (const auto& affectedSkill : affectedSkills)
             {
-                const_cast<skills::FluxioSkill*>(affectedSkill.get())
-                    ->removeSubSkillNodesAndEdges(skillId);
+                affectedSkill->removeSubSkillNodesAndEdges(skillId);
             }
 
             ARMARX_WARNING << "Deleting skill '" << skillIt->second.name << "' (" << skillId
@@ -1129,20 +1133,20 @@ namespace armarx::plugins
         std::shared_lock l(fluxioDC.profilesMutex);
         const auto& profilesEntry = fluxioDC.profiles.find(id);
 
-        if (profilesEntry != fluxioDC.profiles.end())
+        if (profilesEntry == fluxioDC.profiles.end())
         {
             l.unlock();
-            return profilesEntry->second;
+            return std::nullopt;
         }
 
         l.unlock();
-        return std::nullopt;
+        return profilesEntry->second;
     }
 
     skills::FluxioProfile
     SkillManagerComponentPlugin::createProfile(const skills::FluxioProfile& profile)
     {
-        std::string id = IceUtil::generateUUID(); // TODO: use boost library for uuids
+        std::string id = IceUtil::generateUUID();
 
         std::unique_lock l(fluxioDC.profilesMutex);
         fluxioDC.profiles[id] = profile; // a copy is created when the profile is added to the map
@@ -1173,24 +1177,22 @@ namespace armarx::plugins
     {
         const std::string& providerId = boost::uuids::to_string(createUuidWithString(name));
 
-        std::shared_lock providersLock_shared(fluxioDC.providersMutex);
+        std::unique_lock l(fluxioDC.providersMutex);
         if (fluxioDC.providers.find(providerId) != fluxioDC.providers.end())
         {
             ARMARX_WARNING << "Provider with name '" << name << "' already exists.";
             return std::experimental::make_observer(&fluxioDC.providers[providerId]);
-            providersLock_shared.unlock();
+            l.unlock();
             return nullptr;
         }
-        providersLock_shared.unlock();
 
         skills::FluxioProvider p;
         p.id = providerId;
         p.name = name;
 
-        std::unique_lock providersLock_unique(fluxioDC.providersMutex);
         fluxioDC.providers[p.id] = p;
         const auto& ret = std::experimental::make_observer(&fluxioDC.providers[p.id]);
-        providersLock_unique.unlock();
+        l.unlock();
 
         return ret;
     }
@@ -1352,7 +1354,7 @@ namespace armarx::plugins
                                                          const std::string& userId,
                                                          const std::string& skillId)
     {
-        std::shared_lock mutexMapLock_shared(fluxioDC.skillMutexMapMutex);
+        std::scoped_lock l(fluxioDC.skillMutexMapMutex);
         const auto& skillMutexIt = fluxioDC.skillMutexMap.find(skillId);
 
         // aquire mutex
@@ -1361,10 +1363,7 @@ namespace armarx::plugins
             if (skillMutexIt == fluxioDC.skillMutexMap.end())
             {
                 // skill is free, set mutex
-                mutexMapLock_shared.unlock();
-                std::unique_lock mutexMapLock_unique(fluxioDC.skillMutexMapMutex);
                 fluxioDC.skillMutexMap[skillId] = std::make_tuple(userId, armarx::DateTime::Now());
-                mutexMapLock_unique.unlock();
                 return true;
             }
 
@@ -1372,10 +1371,7 @@ namespace armarx::plugins
             if (std::get<0>(fluxioDC.skillMutexMap[skillId]) == userId)
             {
                 // user already holds the mutex -> reset timestamp / prolong mutex
-                mutexMapLock_shared.unlock();
-                std::unique_lock mutexMapLock_unique(fluxioDC.skillMutexMapMutex);
                 std::get<1>(fluxioDC.skillMutexMap[skillId]) = armarx::DateTime::Now();
-                mutexMapLock_unique.unlock();
                 return true;
             }
 
@@ -1385,15 +1381,11 @@ namespace armarx::plugins
             if (elapsedTime.toMinutes() > fluxioDC.mutexTimeout)
             {
                 // mutex invalid, requesting user gets the mutex instead
-                mutexMapLock_shared.unlock();
-                std::unique_lock mutexMapLock_unique(fluxioDC.skillMutexMapMutex);
                 fluxioDC.skillMutexMap[skillId] = std::make_tuple(userId, armarx::DateTime::Now());
-                mutexMapLock_unique.unlock();
                 return true;
             }
 
             // mutex could not be aquired
-            mutexMapLock_shared.unlock();
             return false;
         }
 
@@ -1401,22 +1393,17 @@ namespace armarx::plugins
         if (skillMutexIt == fluxioDC.skillMutexMap.end())
         {
             // skill is already free
-            mutexMapLock_shared.unlock();
             return true;
         }
 
         // check if the user holds the mutex, as only the user that holds the mutex can release it
         if (std::get<0>(fluxioDC.skillMutexMap[skillId]) == userId)
         {
-            mutexMapLock_shared.unlock();
-            std::unique_lock mutexMapLock_unique(fluxioDC.skillMutexMapMutex);
             fluxioDC.skillMutexMap.erase(skillId);
-            mutexMapLock_unique.unlock();
             return true;
         }
 
         // mutex could not be removed
-        mutexMapLock_shared.unlock();
         return false;
     }
 } // namespace armarx::plugins

source/RobotAPI/libraries/skills/manager/SkillManagerComponentPluginUser.cpp

 #include "SkillManagerComponentPluginUser.h"
 
 #include <mutex>
-#include <shared_mutex>
 
 #include <IceUtil/Optional.h>
 
@@ -255,9 +254,9 @@ namespace armarx
                                                  const skills::manager::dto::FluxioSkill& skill,
                                                  const Ice::Current& current)
     {
-        std::shared_lock skillsLock(this->plugin->fluxioDC.skillsMutex);
-        std::shared_lock profilesLock(this->plugin->fluxioDC.profilesMutex);
-        std::shared_lock providersLock(this->plugin->fluxioDC.providersMutex);
+        std::scoped_lock l(this->plugin->fluxioDC.skillsMutex,
+                           this->plugin->fluxioDC.profilesMutex,
+                           this->plugin->fluxioDC.providersMutex);
         auto& skillsMap = this->plugin->fluxioDC.skills;
         auto& providersMap = this->plugin->fluxioDC.providers;
         auto& profilesMap = this->plugin->fluxioDC.profiles;
@@ -268,14 +267,8 @@ namespace armarx
         {
             ARMARX_WARNING << "SkillManagerComponentPluginUser::updateSkill: Skill with id "
                            << skill.id << " not found";
-            skillsLock.unlock();
-            profilesLock.unlock();
-            providersLock.unlock();
             return false;
         }
-        skillsLock.unlock();
-        profilesLock.unlock();
-        providersLock.unlock();
 
         // Check if the user has the mutex for the skill
         if (!this->plugin->getSkillMutex(skill.id, userId))
@@ -285,15 +278,7 @@ namespace armarx
             return false;
         }
 
-
-        profilesLock.lock();
-        providersLock.lock();
-        std::unique_lock skillsLock_unique(this->plugin->fluxioDC.skillsMutex);
         const bool ret = s->second.updateFromIce(skill, providersMap, profilesMap, skillsMap);
-        skillsLock_unique.unlock();
-        profilesLock.unlock();
-        providersLock.unlock();
-
         return ret;
     }
 
@@ -387,11 +372,12 @@ namespace armarx
         const skills::manager::dto::FluxioProfile& profile,
         const Ice::Current& current)
     {
-        std::unique_lock profilesLock(this->plugin->fluxioDC.profilesMutex);
+        std::unique_lock l(this->plugin->fluxioDC.profilesMutex);
         auto& profilesMap = this->plugin->fluxioDC.profiles;
+        l.unlock();
+
         auto ret = this->plugin->createProfile(skills::FluxioProfile::FromIce(profile, profilesMap))
                        .toManagerIce();
-        profilesLock.unlock();
         return ret;
     }
 
@@ -400,10 +386,11 @@ namespace armarx
         const skills::manager::dto::FluxioProfile& profile,
         const Ice::Current& current)
     {
-        std::unique_lock profilesLock(this->plugin->fluxioDC.profilesMutex);
+        std::unique_lock l(this->plugin->fluxioDC.profilesMutex);
         auto& profilesMap = this->plugin->fluxioDC.profiles;
+        l.unlock();
+
         this->plugin->updateProfile(skills::FluxioProfile::FromIce(profile, profilesMap));
-        profilesLock.unlock();
     }
 
     skills::manager::dto::FluxioProviderList
@@ -483,16 +470,15 @@ namespace armarx
         const skills::manager::dto::FluxioSkill& skill,
         const Ice::Current& current)
     {
-        std::unique_lock skillsLock(this->plugin->fluxioDC.skillsMutex);
-        std::shared_lock profilesLock(this->plugin->fluxioDC.profilesMutex);
-        std::shared_lock providersLock(this->plugin->fluxioDC.providersMutex);
+        std::unique_lock skillsLock(this->plugin->fluxioDC.skillsMutex, std::defer_lock);
+        std::unique_lock profilesLock(this->plugin->fluxioDC.profilesMutex, std::defer_lock);
+        std::unique_lock providersLock(this->plugin->fluxioDC.providersMutex, std::defer_lock);
+        std::lock(skillsLock, profilesLock, providersLock);
+
         auto& skillsMap = this->plugin->fluxioDC.skills;
         auto& providersMap = this->plugin->fluxioDC.providers;
         auto& profilesMap = this->plugin->fluxioDC.profiles;
         auto skillBO = skills::FluxioSkill::FromIce(skill, providersMap, profilesMap, skillsMap);
-        skillsLock.unlock();
-        profilesLock.unlock();
-        providersLock.unlock();
 
         if (skillBO == nullptr)
         {
@@ -500,8 +486,11 @@ namespace armarx
             return {};
         }
 
-        auto& skillReleased = *skillBO.release();
+        skillsLock.unlock();
+        profilesLock.unlock();
+        providersLock.unlock();
 
+        auto& skillReleased = *skillBO.release();
         const auto s =
             this->plugin->addSkillToProvider(userId, providerId, std::move(skillReleased));