Merge branch 'fix/skillDeadlock' into 'master'

fix skillmanager and provider deadlocks when callingSubskills, query executionsatus and descriptions and stopping subskills at the same time

See merge request !489

source/RobotAPI/components/skills/SkillProviderExample/CMakeLists.txt

@@ -16,6 +16,7 @@ set(SOURCES
     HelloWorld.cpp
     Incomplete.cpp
     Chaining.cpp
+    ChainingAsync.cpp
     Callback.cpp
     Timeout.cpp
     Segfault.cpp
@@ -30,6 +31,7 @@ set(HEADERS
     HelloWorld.h
     Incomplete.h
     Chaining.h
+    ChainingAsync.h
     Callback.h
     Timeout.h
     Segfault.h

source/RobotAPI/components/skills/SkillProviderExample/Chaining.cpp

@@ -14,9 +14,9 @@ namespace armarx::skills::provider
     {
         return SkillDescription{.skillId = armarx::skills::SkillID{.skillName = "Chaining"},
                                 .description =
-                                    "This skill calls the Timeout skill three times. The last "
+                                    "This skill calls the Timeout skill several times. At some point the "
                                     "execution is aborted due to a timeout of this skill.",
-                                .timeout = armarx::core::time::Duration::MilliSeconds(5000)};
+                                .timeout = armarx::core::time::Duration::MilliSeconds(10000)};
     }
 
     Skill::MainResult
@@ -26,12 +26,13 @@ namespace armarx::skills::provider
             manager,
             skills::SkillID{.providerId = *getSkillId().providerId, .skillName = "Timeout"});
 
-        ARMARX_INFO << "CALL PROXY FIRST TIME";
-        callSubskill(prx);
-        ARMARX_INFO << "CALL PROXY SECOND TIME";
-        callSubskill(prx);
-        ARMARX_INFO << "CALL PROXY THIRD TIME";
-        callSubskill(prx);
+        for (int i = 0; i < 25; i++)
+        {
+            this->throwIfSkillShouldTerminate();
+
+            ARMARX_INFO << "Call subskill number " << i;
+            callSubskill(prx); // Call the Timeout skill and wait for it to finish
+        }
 
         this->throwIfSkillShouldTerminate();
 

source/RobotAPI/components/skills/SkillProviderExample/ChainingAsync.cpp

+#include "ChainingAsync.h"
+
+namespace armarx::skills::provider
+{
+
+    ChainingAsyncSkill::ChainingAsyncSkill() : SimpleSkill(GetSkillDescription())
+    {
+    }
+
+    SkillDescription
+    ChainingAsyncSkill::GetSkillDescription()
+    {
+        return SkillDescription{.skillId = armarx::skills::SkillID{.skillName = "ChainingAsync"},
+                                .description =
+                                    "This skill calls the Timeout skill several times async.",
+                                .timeout = armarx::core::time::Duration::MilliSeconds(5000)};
+    }
+
+    Skill::MainResult
+    ChainingAsyncSkill::main(const MainInput& in)
+    {
+        SkillProxy prx(
+            manager,
+            skills::SkillID{.providerId = *getSkillId().providerId, .skillName = "Timeout"});
+
+        for (int i = 0; i < 25; i++)
+        {
+            this->throwIfSkillShouldTerminate();
+
+            ARMARX_INFO << "Call subskill number " << i;
+            callSubskillAsync(prx); // Call the Timeout skill
+        }
+
+        this->throwIfSkillShouldTerminate();
+
+        return {TerminatedSkillStatus::Succeeded, nullptr};
+    }
+} // namespace armarx::skills::provider

source/RobotAPI/components/skills/SkillProviderExample/ChainingAsync.h

+
+/*
+ * This file is part of ArmarX.
+ *
+ * ArmarX is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * ArmarX is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * @author     Fabian Reister ( fabian dot reister at kit dot edu )
+ * @date       2021
+ * @copyright  http://www.gnu.org/licenses/gpl-2.0.txt
+ *             GNU General Public License
+ */
+
+#pragma once
+
+// RobotAPI
+#include <RobotAPI/libraries/skills/provider/SimpleSkill.h>
+
+namespace armarx::skills::provider
+{
+    class ChainingAsyncSkill : public SimpleSkill
+    {
+    public:
+        ChainingAsyncSkill();
+
+        static SkillDescription GetSkillDescription();
+
+    private:
+        Skill::MainResult main(const MainInput&) final;
+    };
+} // namespace armarx::skills::provider

source/RobotAPI/components/skills/SkillProviderExample/InstantKill.cpp

@@ -31,7 +31,7 @@ namespace armarx::skills::provider
           manager,
           skills::SkillID{.providerId = *getSkillId().providerId, .skillName = "Chaining"});
 
-      for (unsigned int i = 0; i < 10; ++i)
+      for (unsigned int i = 0; i < 25; ++i)
       {
         auto id = callSubskillAsync(prx);
         prx.abortSkillAsync(id);

source/RobotAPI/components/skills/SkillProviderExample/SkillProviderExample.cpp

@@ -54,6 +54,10 @@ namespace armarx::skills::provider
         ARMARX_INFO << "Adding skill ChainingSkill";
         addSkillFactory<ChainingSkill>();
 
+        // chainingAsync
+        ARMARX_INFO << "Adding skill ChainingAsyncSkill";
+        addSkillFactory<ChainingAsyncSkill>();
+
         // incomplete and prepare
         ARMARX_INFO << "Adding skill IncompleteSkill";
         addSkillFactory<IncompleteSkill>();

source/RobotAPI/components/skills/SkillProviderExample/SkillProviderExample.h

@@ -31,6 +31,7 @@
 
 #include "Callback.h"
 #include "Chaining.h"
+#include "ChainingAsync.h"
 #include "HelloWorld.h"
 #include "Incomplete.h"
 #include "Segfault.h"

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

@@ -332,11 +332,15 @@ namespace armarx::plugins
 
             try
             {
-                auto async = provider->begin_abortSkill(executionId.toProviderIce());
-                return true;
+                auto async = provider->begin_abortSkillAsync(executionId.toProviderIce());
+                l.unlock(); // allow parallel e.g. stopping. Otherwise the manager would lock himself in nested calls
+                auto r = provider->end_abortSkillAsync(async);
+                return r.success;
             }
             catch (const std::exception &e)
             {
+                l.lock();
+
                 handleExceptionNonLocking(__PRETTY_FUNCTION__, e, providerId);
 
                 return false;
@@ -392,6 +396,8 @@ namespace armarx::plugins
             }
             catch (const std::exception &e)
             {
+                l.lock();
+
                 handleExceptionNonLockingThrow(__PRETTY_FUNCTION__, e, providerId);
             }
         }
@@ -406,7 +412,7 @@ namespace armarx::plugins
     {
         std::map<skills::SkillID, skills::SkillDescription> ret;
 
-        std::scoped_lock l(skillProviderMapMutex);
+        std::unique_lock l(skillProviderMapMutex);
         for (auto it = skillProviderMap.cbegin(); it != skillProviderMap.cend();)
         {
             const auto& providerId = it->first;
@@ -422,7 +428,9 @@ namespace armarx::plugins
 
             try
             {
-                skills::provider::dto::SkillDescriptionMap m = provider->getSkillDescriptions();
+                auto async = provider->begin_getSkillDescriptions();
+                l.unlock();
+                auto m = provider->end_getSkillDescriptions(async);
 
                 for (const auto& [provider_skillId_ice, skillDescription_ice] : m)
                 {
@@ -435,6 +443,8 @@ namespace armarx::plugins
             }
             catch (const std::exception &e)
             {
+                l.lock();
+
                 if (auto _it = handleExceptionNonLocking(__PRETTY_FUNCTION__, e, providerId))
                 {
                     it = _it.value(); // next element
@@ -505,7 +515,7 @@ namespace armarx::plugins
     {
         std::map<skills::SkillExecutionID, skills::SkillStatusUpdate> ret;
 
-        std::scoped_lock l(skillProviderMapMutex);
+        std::unique_lock l(skillProviderMapMutex);
         for (auto it = skillProviderMap.cbegin(); it != skillProviderMap.cend();)
         {
             const auto& providerId = it->first;
@@ -521,7 +531,9 @@ namespace armarx::plugins
 
             try
             {
-                auto m = provider->getSkillExecutionStatuses();
+                auto async = provider->begin_getSkillExecutionStatuses();
+                l.unlock(); // allow parallel e.g. stopping. Otherwise the manager would lock himself in nested calls
+                auto m = provider->end_getSkillExecutionStatuses(async);
 
                 for (const auto& [provider_executionId_ice, provider_statusUpdate_ice] : m)
                 {

source/RobotAPI/libraries/skills/provider/SkillProviderComponentPlugin.cpp

@@ -315,6 +315,7 @@ namespace armarx::plugins
                 {
                     break;
                 }
+
             }
 
             std::this_thread::sleep_for(std::chrono::milliseconds(20));
@@ -370,7 +371,7 @@ namespace armarx::plugins
         while (true)
         {
             {
-                std::scoped_lock l(runtime.skillStatusesMutex);
+                std::scoped_lock l2(runtime.skillStatusesMutex);
                 auto status = runtime.statusUpdate;
 
                 if (status.hasBeenTerminated())