From abf7d059884dcaec9d8553a4b6f649829643e25a Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 10 Nov 2020 14:16:57 -0600 Subject: [PATCH 1/5] Test demonstrating segfault The segfault occurs when a model containing a Breadcrumbs system gets unloaded by the level manager. Since systems don't get unloaded, the Breadcrumbs system continues to operate assuming that the model it is associated with is valid. Signed-off-by: Addisu Z. Taddese --- test/integration/breadcrumbs.cc | 99 ++++++++++++++++++++++++ test/worlds/breadcrumbs_levels.sdf | 120 +++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+) create mode 100644 test/worlds/breadcrumbs_levels.sdf diff --git a/test/integration/breadcrumbs.cc b/test/integration/breadcrumbs.cc index 35ba3c8003..4fb6a97bb7 100644 --- a/test/integration/breadcrumbs.cc +++ b/test/integration/breadcrumbs.cc @@ -26,6 +26,7 @@ #include #include +#include "ignition/gazebo/Entity.hh" #include "ignition/gazebo/Server.hh" #include "ignition/gazebo/SystemLoader.hh" #include "ignition/gazebo/components/Name.hh" @@ -547,6 +548,104 @@ TEST_F(BreadcrumbsTest, AllowRenaming) EXPECT_TRUE(this->server->HasEntity("B1_0_1")); } +///////////////////////////////////////////////// +// The test checks that models containing Breadcrumbs can be unloaded and loaded +// safely +TEST_F(BreadcrumbsTest, LevelLoadUnload) +{ + // Start server + this->LoadWorld("test/worlds/breadcrumbs_levels.sdf", true); + + test::Relay testSystem; + transport::Node node; + auto deploy = + node.Advertise("/model/tile_1/breadcrumbs/B1/deploy"); + + const std::size_t iterTestStart = 1000; + const std::size_t nIters = iterTestStart + 10000; + + std::optional initialPose; + testSystem.OnPostUpdate( + [&](const gazebo::UpdateInfo &_info, + const gazebo::EntityComponentManager &_ecm) + { + // Ensure that tile_1 is loaded at the start, deploy a breadcrumb and + // check if the breadcrumb is deployed + if (_info.iterations == iterTestStart) + { + auto modelEntity = _ecm.EntityByComponents( + components::Model(), components::Name("tile_1")); + EXPECT_NE(kNullEntity, modelEntity); + EXPECT_TRUE(deploy.Publish(msgs::Empty())); + } + else if (_info.iterations == iterTestStart + 1000) + { + auto deployedB1 = _ecm.EntityByComponents(components::Model(), + components::Name("B1_0")); + EXPECT_NE(kNullEntity, deployedB1); + } + // Move the performer (sphere) outside the level and ensure that tile_1 + // is unloaded + else if (_info.iterations == iterTestStart + 1001) + { + msgs::Pose req; + req.set_name("sphere"); + req.mutable_position()->set_x(30); + msgs::Boolean rep; + bool result; + node.Request("/world/breadcrumbs_levels/set_pose", req, 1000, rep, + result); + EXPECT_TRUE(result); + } + // Check that tile_1 is unloaded, then try deploying breadcrumb. This + // should fail because the model associated with the breadcrumb system + // is unloaded. + else if (_info.iterations == iterTestStart + 2000) + { + auto modelEntity = _ecm.EntityByComponents( + components::Model(), components::Name("tile_1")); + EXPECT_EQ(kNullEntity, modelEntity); + EXPECT_TRUE(deploy.Publish(msgs::Empty())); + } + else if (_info.iterations == iterTestStart + 3000) + { + auto deployedB1 = _ecm.EntityByComponents(components::Model(), + components::Name("B1_1")); + EXPECT_EQ(kNullEntity, deployedB1); + } + // Move the performer (sphere) back into the level and ensure that + // tile_1 is loaded + else if (_info.iterations == iterTestStart + 3001) + { + msgs::Pose req; + req.set_name("sphere"); + req.mutable_position()->set_x(0); + msgs::Boolean rep; + bool result; + node.Request("/world/breadcrumbs_levels/set_pose", req, 1000, rep, + result); + EXPECT_TRUE(result); + } + // Check that tile_1 is loaded, then try deploying breadcrumb. + else if (_info.iterations == iterTestStart + 4000) + { + auto modelEntity = _ecm.EntityByComponents( + components::Model(), components::Name("tile_1")); + EXPECT_NE(kNullEntity, modelEntity); + EXPECT_TRUE(deploy.Publish(msgs::Empty())); + } + else if (_info.iterations == iterTestStart + 5000) + { + auto deployedB1 = _ecm.EntityByComponents(components::Model(), + components::Name("B1_0_1")); + EXPECT_NE(kNullEntity, deployedB1); + } + }); + + this->server->AddSystem(testSystem.systemPtr); + this->server->Run(true, nIters, false); +} + ///////////////////////////////////////////////// /// Main int main(int _argc, char **_argv) diff --git a/test/worlds/breadcrumbs_levels.sdf b/test/worlds/breadcrumbs_levels.sdf new file mode 100644 index 0000000000..d0f83e52b3 --- /dev/null +++ b/test/worlds/breadcrumbs_levels.sdf @@ -0,0 +1,120 @@ + + + + + + 0.001 + 1.0 + + + + + + + + + + 0 0 0.1 0 0 0 + true + + + + + 10 10 0.2 + + + + + + + 10 10 0.2 + + + + 0.8 0.8 0.8 1 + 0.0 0.8 0.0 1 + 0.8 0.8 0.8 1 + + + + + 3 + 15 + true + + + + 0 0 5 0 0 0 + + + + + 0.3 0.3 0.5 + + + + + + + 0.3 0.3 0.5 + + + + + + + + + + + + 0 0 2 0 0 0 + true + + + + + 1.0 + + + + + + + 1.0 + + + + + + + + + sphere + + + 4 4 2 + + + + + 0 0 2.5 0 0 0 + + + 10 10 10 + + + 2 + tile_1 + + + + + + From d66f16a820e7c74d19a3fdb5c6bd1692fa51cdcc Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 10 Nov 2020 15:27:57 -0600 Subject: [PATCH 2/5] Improve test to look for breadcrumbs whose names match a regex. Signed-off-by: Addisu Z. Taddese --- test/integration/breadcrumbs.cc | 71 +++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 25 deletions(-) diff --git a/test/integration/breadcrumbs.cc b/test/integration/breadcrumbs.cc index 4fb6a97bb7..ac5e80db6b 100644 --- a/test/integration/breadcrumbs.cc +++ b/test/integration/breadcrumbs.cc @@ -17,6 +17,8 @@ #include +#include + #include #include @@ -549,6 +551,25 @@ TEST_F(BreadcrumbsTest, AllowRenaming) } ///////////////////////////////////////////////// +/// Return a list of model entities whose names match the given regex +std::vector ModelsByNameRegex( + const gazebo::EntityComponentManager &_ecm, const std::regex &_re) +{ + std::vector entities; + _ecm.Each( + [&](const Entity _entity, const components::Model *, + const components::Name *_name) + { + if (std::regex_match(_name->Data(), _re)) + { + entities.push_back(_entity); + } + return true; + }); + + return entities; +} + // The test checks that models containing Breadcrumbs can be unloaded and loaded // safely TEST_F(BreadcrumbsTest, LevelLoadUnload) @@ -564,28 +585,27 @@ TEST_F(BreadcrumbsTest, LevelLoadUnload) const std::size_t iterTestStart = 1000; const std::size_t nIters = iterTestStart + 10000; - std::optional initialPose; + std::regex reTile1{"tile_1"}; + std::regex reBreadcrumb{"B1_.*"}; testSystem.OnPostUpdate( [&](const gazebo::UpdateInfo &_info, const gazebo::EntityComponentManager &_ecm) { - // Ensure that tile_1 is loaded at the start, deploy a breadcrumb and - // check if the breadcrumb is deployed + // Ensure that tile_1 is loaded at the start, deploy a breadcrumb if (_info.iterations == iterTestStart) { - auto modelEntity = _ecm.EntityByComponents( - components::Model(), components::Name("tile_1")); - EXPECT_NE(kNullEntity, modelEntity); + auto tiles = ModelsByNameRegex(_ecm, reTile1); + EXPECT_EQ(1u, tiles.size()); EXPECT_TRUE(deploy.Publish(msgs::Empty())); } + //check if the breadcrumb has been deployed else if (_info.iterations == iterTestStart + 1000) { - auto deployedB1 = _ecm.EntityByComponents(components::Model(), - components::Name("B1_0")); - EXPECT_NE(kNullEntity, deployedB1); + auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); + EXPECT_EQ(1u, breadcrumbs.size()); } - // Move the performer (sphere) outside the level and ensure that tile_1 - // is unloaded + // Move the performer (sphere) outside the level. This should cause + // tile_1 to be unloaded else if (_info.iterations == iterTestStart + 1001) { msgs::Pose req; @@ -602,19 +622,18 @@ TEST_F(BreadcrumbsTest, LevelLoadUnload) // is unloaded. else if (_info.iterations == iterTestStart + 2000) { - auto modelEntity = _ecm.EntityByComponents( - components::Model(), components::Name("tile_1")); - EXPECT_EQ(kNullEntity, modelEntity); + auto tiles = ModelsByNameRegex(_ecm, reTile1); + EXPECT_EQ(0u, tiles.size()); EXPECT_TRUE(deploy.Publish(msgs::Empty())); } + // Check that no new breadcrumbs have been deployed else if (_info.iterations == iterTestStart + 3000) { - auto deployedB1 = _ecm.EntityByComponents(components::Model(), - components::Name("B1_1")); - EXPECT_EQ(kNullEntity, deployedB1); + auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); + EXPECT_EQ(1u, breadcrumbs.size()); } - // Move the performer (sphere) back into the level and ensure that - // tile_1 is loaded + // Move the performer (sphere) back into the level. This should load + // tile_1 and a new Breadcrumbs system. else if (_info.iterations == iterTestStart + 3001) { msgs::Pose req; @@ -629,16 +648,18 @@ TEST_F(BreadcrumbsTest, LevelLoadUnload) // Check that tile_1 is loaded, then try deploying breadcrumb. else if (_info.iterations == iterTestStart + 4000) { - auto modelEntity = _ecm.EntityByComponents( - components::Model(), components::Name("tile_1")); - EXPECT_NE(kNullEntity, modelEntity); + auto tiles = ModelsByNameRegex(_ecm, reTile1); + EXPECT_EQ(1u, tiles.size()); EXPECT_TRUE(deploy.Publish(msgs::Empty())); } + // Check that only one additional breadcrumb has been deployed even + // though there are two Breadcrumbs systems associated with the model. + // The original Breadcrumbs system should now be invalid because the + // model gets a new Entity ID. else if (_info.iterations == iterTestStart + 5000) { - auto deployedB1 = _ecm.EntityByComponents(components::Model(), - components::Name("B1_0_1")); - EXPECT_NE(kNullEntity, deployedB1); + auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); + EXPECT_EQ(2u, breadcrumbs.size()); } }); From 59c8a19e04734f80149d8a1abad43b0041eca590 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 10 Nov 2020 15:28:51 -0600 Subject: [PATCH 3/5] Check if model is valid in before trying to retrieve its pose. Signed-off-by: Addisu Z. Taddese --- src/systems/breadcrumbs/Breadcrumbs.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/systems/breadcrumbs/Breadcrumbs.cc b/src/systems/breadcrumbs/Breadcrumbs.cc index f9dd83e497..57d0e73338 100644 --- a/src/systems/breadcrumbs/Breadcrumbs.cc +++ b/src/systems/breadcrumbs/Breadcrumbs.cc @@ -157,6 +157,15 @@ void Breadcrumbs::PreUpdate(const ignition::gazebo::UpdateInfo &_info, std::back_inserter(cmds)); this->pendingCmds.clear(); } + // Check that the model is valid before continuing. This check is needed + // because the model associated with the Breadcrumbs might have been + // unloaded by the level manager. Ideally, this system would have been + // unloaded along with the model, but that is not currently the case. See + // issue #113 + if (!model.Valid(_ecm)) + { + return; + } auto poseComp = _ecm.Component(this->model.Entity()); From 59b39984ccdac785810ee864667ad9a690273a32 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 10 Nov 2020 15:43:51 -0600 Subject: [PATCH 4/5] Codecheck Signed-off-by: Addisu Z. Taddese --- test/integration/breadcrumbs.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/breadcrumbs.cc b/test/integration/breadcrumbs.cc index ac5e80db6b..40e1402da5 100644 --- a/test/integration/breadcrumbs.cc +++ b/test/integration/breadcrumbs.cc @@ -17,11 +17,11 @@ #include -#include - #include #include +#include + #include #include @@ -598,7 +598,7 @@ TEST_F(BreadcrumbsTest, LevelLoadUnload) EXPECT_EQ(1u, tiles.size()); EXPECT_TRUE(deploy.Publish(msgs::Empty())); } - //check if the breadcrumb has been deployed + // Check if the breadcrumb has been deployed else if (_info.iterations == iterTestStart + 1000) { auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); From 9fa5c76c742c7890745bbbaee2c58c5a46773d15 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 10 Nov 2020 18:25:58 -0600 Subject: [PATCH 5/5] Check if model is valid during Configure Signed-off-by: Addisu Z. Taddese --- src/systems/breadcrumbs/Breadcrumbs.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/systems/breadcrumbs/Breadcrumbs.cc b/src/systems/breadcrumbs/Breadcrumbs.cc index 57d0e73338..71388fccf1 100644 --- a/src/systems/breadcrumbs/Breadcrumbs.cc +++ b/src/systems/breadcrumbs/Breadcrumbs.cc @@ -71,6 +71,12 @@ void Breadcrumbs::Configure(const Entity &_entity, _sdf->Get("allow_renaming", this->allowRenaming).first; this->model = Model(_entity); + if (!this->model.Valid(_ecm)) + { + ignerr << "The Breadcrumbs system should be attached to a model entity. " + << "Failed to initialize." << std::endl; + return; + } if (!_sdf->HasElement("breadcrumb")) { @@ -162,7 +168,7 @@ void Breadcrumbs::PreUpdate(const ignition::gazebo::UpdateInfo &_info, // unloaded by the level manager. Ideally, this system would have been // unloaded along with the model, but that is not currently the case. See // issue #113 - if (!model.Valid(_ecm)) + if (!this->model.Valid(_ecm)) { return; }