From 040ec7fed084fbb82618dcfc00abef596c9db3a1 Mon Sep 17 00:00:00 2001
From: Fabian Peller-Konrad <fabian.peller-konrad@kit.edu>
Date: Wed, 2 Oct 2024 16:16:50 +0200
Subject: [PATCH] fix skill memory error, where the skillProvidersMutex was
unlocked multiple time. Its now relocked in the for loops
clang
Signed-off-by: Fabian Peller-Konrad <fabian.peller-konrad@kit.edu>
---
.../manager/SkillManagerComponentPlugin.cpp | 57 ++++++++++---------
1 file changed, 31 insertions(+), 26 deletions(-)
diff --git a/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp b/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp
index fbecf04e5..0f53619d6 100644
--- a/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp
+++ b/source/RobotAPI/libraries/skills/manager/SkillManagerComponentPlugin.cpp
@@ -96,8 +96,6 @@ namespace armarx::plugins
{
provderId = getFirstProviderNameThatHasSkill(executionRequest.skillId);
}
-
-
if (auto it = skillProviderMap.find(provderId); it != skillProviderMap.end())
{
const auto& provider = it->second;
@@ -132,7 +130,7 @@ namespace armarx::plugins
skills::SkillStatusUpdate::FromIce(provider_statusUpdate_ice, provderId);
return statusUpdate;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -201,7 +199,7 @@ namespace armarx::plugins
executionId.skillId.providerId = provderId;
return executionId;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -249,7 +247,7 @@ namespace armarx::plugins
auto r = provider->end_updateSkillParameters(async);
return r.success;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -294,7 +292,7 @@ namespace armarx::plugins
auto r = provider->end_abortSkill(async);
return r.success;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -337,7 +335,7 @@ namespace armarx::plugins
auto r = provider->end_abortSkillAsync(async);
return r.success;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -394,7 +392,7 @@ namespace armarx::plugins
skills::SkillDescription::FromIce(provider_desc_ice.value(), providerId);
return desc;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -431,6 +429,7 @@ namespace armarx::plugins
auto async = provider->begin_getSkillDescriptions();
l.unlock();
auto m = provider->end_getSkillDescriptions(async);
+ l.lock();
for (const auto& [provider_skillId_ice, skillDescription_ice] : m)
{
@@ -441,7 +440,7 @@ namespace armarx::plugins
++it;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -497,7 +496,7 @@ namespace armarx::plugins
provider_statusUpdate_ice.value(), providerId);
return statusUpdate;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
l.lock();
@@ -534,6 +533,7 @@ namespace armarx::plugins
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);
+ l.lock();
for (const auto& [provider_executionId_ice, provider_statusUpdate_ice] : m)
{
@@ -544,7 +544,7 @@ namespace armarx::plugins
}
it++;
}
- catch (const std::exception &e)
+ catch (const std::exception& e)
{
if (auto _it = handleExceptionNonLocking(__PRETTY_FUNCTION__, e, providerId))
{
@@ -555,32 +555,37 @@ namespace armarx::plugins
return ret;
}
- std::optional<std::map<skills::ProviderID, skills::provider::dti::SkillProviderInterfacePrx>::iterator>
- SkillManagerComponentPlugin::handleExceptionNonLocking(
- const char* funcName, const std::exception& e, skills::ProviderID providerId, bool eraseSkillProvider)
+ std::optional<
+ std::map<skills::ProviderID, skills::provider::dti::SkillProviderInterfacePrx>::iterator>
+ SkillManagerComponentPlugin::handleExceptionNonLocking(const char* funcName,
+ const std::exception& e,
+ skills::ProviderID providerId,
+ bool eraseSkillProvider)
{
// NON LOCKING! WE ASSERT THAT THE CALLER HOLDS LOCK
- if (auto it = skillProviderMap.find(providerId); eraseSkillProvider and it != skillProviderMap.end())
+ if (auto it = skillProviderMap.find(providerId);
+ eraseSkillProvider and it != skillProviderMap.end())
{
- ARMARX_WARNING << funcName
- << ": Found disconnected or buggy skill provider '" << providerId
- << "' during execution. Removing it from skills. "
- << "Error: " << e.what();
+ ARMARX_WARNING << funcName << ": Found disconnected or buggy skill provider '"
+ << providerId << "' during execution. Removing it from skills. "
+ << "Error: " << e.what();
return skillProviderMap.erase(it);
}
else
{
- ARMARX_WARNING << funcName
- << ": Found disconnected or buggy skill provider '" << providerId
- << "' during execution. However, it already got removed... "
- << "Error: " << e.what();
+ ARMARX_WARNING << funcName << ": Found disconnected or buggy skill provider '"
+ << providerId
+ << "' during execution. However, it already got removed... "
+ << "Error: " << e.what();
return std::nullopt;
}
}
- void
- SkillManagerComponentPlugin::handleExceptionNonLockingThrow(
- const char* funcName, const std::exception& e, skills::ProviderID providerId, bool eraseSkillProvider)
+ void
+ SkillManagerComponentPlugin::handleExceptionNonLockingThrow(const char* funcName,
+ const std::exception& e,
+ skills::ProviderID providerId,
+ bool eraseSkillProvider)
{
// NON LOCKING! WE ASSERT THAT THE CALLER HOLDS LOCK
handleExceptionNonLocking(funcName, e, providerId, eraseSkillProvider);
--
GitLab