Skip to content
Snippets Groups Projects
Commit c3c6fdc9 authored by Fabian Tërnava's avatar Fabian Tërnava
Browse files

fixed bug which led to segmentation fault when instantiating a skill from a detached thread

parent e2abd2b0
No related branches found
No related tags found
1 merge request!388Refactor skills framework
Pipeline #15613 passed
Stage: build-and-test
Hide whitespace changes
Inline Side-by-side
Showing
with 72 additions and 61 deletions
source/RobotAPI/libraries/skills/provider/SkillFactory.h
+ 16
− 9
View file @ c3c6fdc9
......@@ -11,12 +11,18 @@ namespace armarx
class SkillBlueprint
{
public:
using FunTSkill = std::function<std::unique_ptr<Skill>()>;
using FunctionTypeToCreateSkill = std::function<std::unique_ptr<Skill>()>;
SkillBlueprint(const FunTSkill& s) : createS(s)
SkillBlueprint(const FunctionTypeToCreateSkill& s) : _createSkill(s)
{
}
SkillBlueprint(FunctionTypeToCreateSkill&& s) : _createSkill(std::move(s))
{
}
SkillBlueprint(const SkillBlueprint&) = default;
virtual ~SkillBlueprint() = default;
template <class _Skill, class... Args>
......@@ -25,19 +31,20 @@ namespace armarx
static std::unique_ptr<SkillBlueprint>
ForSkill(Args&&... args)
{
// capture params in new lambda
// (https://stackoverflow.com/questions/47496358/c-lambdas-how-to-capture-variadic-parameter-pack-from-the-upper-scope)
auto createS = [... args = std::forward<Args>(args)]()
{ return std::make_unique<_Skill>(args...); };
auto _createSkill = [=]()
{
auto ptr = std::make_unique<_Skill>(args...);
return ptr;
};
auto ret = std::make_unique<SkillBlueprint>(createS);
auto ret = std::make_unique<SkillBlueprint>(std::move(_createSkill));
return ret;
}
virtual std::unique_ptr<Skill>
createSkill(const skills::ProviderID& pid) const
{
auto s = createS();
auto s = _createSkill();
s->setProviderId(pid);
return s;
}
......@@ -51,7 +58,7 @@ namespace armarx
protected:
FunTSkill createS;
FunctionTypeToCreateSkill _createSkill;
};
template <class T>
......
source/RobotAPI/libraries/skills/provider/SkillProviderComponentPlugin.cpp
+ 46
− 47
View file @ c3c6fdc9
......@@ -174,42 +174,40 @@ namespace armarx::plugins
skills::SkillStatusUpdate
SkillProviderComponentPlugin::executeSkill(
const skills::SkillExecutionID& executionId,
const aron::data::DictPtr& parameters,
const skills::callback::dti::SkillProviderCallbackInterfacePrx& callbackInterface)
const skills::SkillExecutionRequest& executionRequest)
{
skills::SkillStatusUpdate ret{executionId, parameters, callbackInterface};
ARMARX_CHECK(executionRequest.skillId.isFullySpecified());
std::thread execution;
skills::SkillExecutionID executionId(executionRequest.skillId,
executionRequest.executorName,
armarx::core::time::DateTime::Now());
skills::SkillStatusUpdate ret{
executionId, executionRequest.parameters, executionRequest.callbackInterface};
skills::detail::SkillRuntime* wrapper;
{
auto l1 = std::unique_lock{skillFactoriesMutex};
const auto& fac = getSkillFactory(executionId.skillId);
ARMARX_CHECK(fac) << "Could not find a factory for skill " << executionId.skillId;
{
const std::unique_lock l2{skillExecutionsMutex};
auto it =
skillExecutions.emplace(std::piecewise_construct,
std::make_tuple(executionId),
std::make_tuple(*fac,
executionId,
executionRequest.parameters,
executionRequest.callbackInterface));
wrapper = &it.first->second;
}
// async start execution. But we wait for the execution to finish at the end of this method
execution = std::thread(
wrapper->execution = std::thread(
[&]()
{
skills::detail::SkillRuntime* wrapper;
{
const std::unique_lock l2{skillExecutionsMutex};
auto it = skillExecutions.emplace(
std::piecewise_construct,
std::make_tuple(executionId),
std::make_tuple(*fac, executionId, parameters, callbackInterface));
ARMARX_CHECK(it.second)
<< "For some reason a skill '" << executionId.skillId.toString()
<< "' execution failed when instantiating the "
"wrapper class and adding it to the internally used map. This "
"should "
"not happen!!";
wrapper = &it.first->second;
}
// execute waits until the previous execution finishes.
auto x = wrapper->executeSkill();
ret.result = x.result;
......@@ -217,9 +215,9 @@ namespace armarx::plugins
});
} // release lock. We don't know how long the skill needs to finish and we have to release the lock for being able to abort the execution
if (execution.joinable())
if (wrapper && wrapper->execution.joinable())
{
execution.join();
wrapper->execution.join();
// tidy up map of executions
const std::unique_lock l2{skillExecutionsMutex};
......@@ -231,20 +229,6 @@ namespace armarx::plugins
return ret;
}
skills::SkillStatusUpdate
SkillProviderComponentPlugin::executeSkill(
const skills::SkillExecutionRequest& executionRequest)
{
ARMARX_CHECK(executionRequest.skillId.isFullySpecified());
skills::SkillExecutionID executionId(executionRequest.skillId,
executionRequest.executorName,
armarx::core::time::DateTime::Now());
return this->executeSkill(
executionId, executionRequest.parameters, executionRequest.callbackInterface);
}
skills::SkillExecutionID
SkillProviderComponentPlugin::executeSkillAsync(
const skills::SkillExecutionRequest& executionRequest)
......@@ -255,18 +239,33 @@ namespace armarx::plugins
executionRequest.executorName,
armarx::core::time::DateTime::Now());
std::thread execution;
skills::detail::SkillRuntime* wrapper;
{
execution = std::thread(
auto l1 = std::unique_lock{skillFactoriesMutex};
const auto& fac = getSkillFactory(executionId.skillId);
ARMARX_CHECK(fac) << "Could not find a factory for skill " << executionId.skillId;
{
const std::unique_lock l2{skillExecutionsMutex};
auto it =
skillExecutions.emplace(std::piecewise_construct,
std::make_tuple(executionId),
std::make_tuple(*fac,
executionId,
executionRequest.parameters,
executionRequest.callbackInterface));
wrapper = &it.first->second;
}
wrapper->execution = std::thread(
[&]()
{
this->executeSkill(executionId,
executionRequest.parameters,
executionRequest.callbackInterface);
// execute waits until the previous execution finishes.
auto x = wrapper->executeSkill();
});
}
execution.detach();
return executionId;
}
......
source/RobotAPI/libraries/skills/provider/SkillProviderComponentPlugin.h
+ 0
− 4
View file @ c3c6fdc9
......@@ -85,10 +85,6 @@ namespace armarx::plugins
std::optional<skills::SkillDescription> getSkillDescription(const skills::SkillID&) const;
std::map<skills::SkillID, skills::SkillDescription> getSkillDescriptions() const;
skills::SkillStatusUpdate executeSkill(
const skills::SkillExecutionID& executionId,
const aron::data::DictPtr& parameters,
const skills::callback::dti::SkillProviderCallbackInterfacePrx& callbackInterface);
skills::SkillStatusUpdate executeSkill(const skills::SkillExecutionRequest& executionInfo);
skills::SkillExecutionID
......
source/RobotAPI/libraries/skills/provider/detail/SkillImplementationWrapper.cpp
+ 6
− 0
View file @ c3c6fdc9
......@@ -115,6 +115,8 @@ namespace armarx
// -------------------------------------------------------------------------------------
// construct skill
// -------------------------------------------------------------------------------------
ARMARX_INFO_S << "Construct skill: " << skillName;
updateStatus(SkillStatus::Constructing);
this->skill = this->factory.createSkill(providerId);
this->skill->executorName = executorName;
......@@ -182,6 +184,7 @@ namespace armarx
// -------------------------------------------------------------------------------------
// Init skill
// -------------------------------------------------------------------------------------
ARMARX_INFO_S << "Init skill: " << skillName;
updateStatus(SkillStatus::Initializing);
try
......@@ -215,6 +218,7 @@ namespace armarx
// -------------------------------------------------------------------------------------
// Prepare skill
// -------------------------------------------------------------------------------------
ARMARX_INFO_S << "Prepare skill: " << skillName;
updateStatus(SkillStatus::Preparing);
try
......@@ -260,6 +264,7 @@ namespace armarx
// Main of skill
// -------------------------------------------------------------------------------------
// execute. If the skill fails for some reason, from this point it will always execute its exit function.
ARMARX_INFO_S << "Main skill: " << skillName;
updateStatus(SkillStatus::Running);
Skill::MainResult mainRet;
......@@ -294,6 +299,7 @@ namespace armarx
// -------------------------------------------------------------------------------------
// Exit of skill
// -------------------------------------------------------------------------------------
ARMARX_INFO_S << "Exit skill: " << skillName;
try
{
Skill::ExitResult exitRet = skill->exitSkill();
......
source/RobotAPI/libraries/skills/provider/detail/SkillImplementationWrapper.h
+ 4
− 1
View file @ c3c6fdc9
......@@ -18,7 +18,7 @@ namespace armarx
class SkillRuntime
{
private:
const skills::SkillBlueprint& factory;
const skills::SkillBlueprint factory;
std::unique_ptr<Skill> skill;
mutable std::mutex executionMutex;
......@@ -29,6 +29,9 @@ namespace armarx
mutable std::mutex skillStatusesMutex;
SkillStatusUpdate statusUpdate;
// Set exteranally to store the execution somewhere
std::thread execution;
// ctor
SkillRuntime(const skills::SkillBlueprint& fac,
const skills::SkillExecutionID&,
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment