diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 9843f40ee..b32f1323d 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -154,7 +154,7 @@ class Tree */ [[nodiscard]] std::shared_ptr wakeUpSignal() const; - ~Tree(); + ~Tree() noexcept; /// Tick the root of the tree once, even if a node invoked /// emitWakeUpSignal() diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index e3e376c50..2f0166d9b 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -718,9 +718,19 @@ std::shared_ptr Tree::wakeUpSignal() const return wake_up_; } -Tree::~Tree() +Tree::~Tree() noexcept { - haltTree(); + // A destructor that lets an exception escape calls std::terminate, and a + // node is allowed to throw while halting - TryCatch with catch_on_halt + // ticks its catch child, and ticking may throw. Swallow it: there is no + // caller left to report to, and the tree is being destroyed anyway. + try + { + haltTree(); + } + catch(...) + { + } } NodeStatus Tree::tickExactlyOnce() diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 165dacab6..55567fa4c 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -253,15 +253,27 @@ struct XMLParser::PImpl // it would otherwise remain the "first opened document" that // instantiateTree() reads main_tree_to_execute from, and a document that // did not parse has no root element at all. + // + // Rollback is per load call, not per document: makes one call + // register trees from several documents, and if the root document is + // rejected the included ones have already registered theirs. Restore the + // state from before the call, which also puts back a tree that this call + // redefined - its element lives in the document about to be erased. void loadLastDocImpl(bool add_includes) { const auto doc_it = std::prev(opened_documents.end()); + const auto roots_before = tree_roots; + const auto sources_before = tree_sources; + const int suffix_before = suffix_count; try { loadDocImpl(doc_it->get(), add_includes); } catch(...) { + tree_roots = roots_before; + tree_sources = sources_before; + suffix_count = suffix_before; opened_documents.erase(doc_it); throw; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5d4e643c9..31c9aa9d0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -61,6 +61,8 @@ set(BT_TESTS gtest_plugin_issue953.cpp gtest_blackboard_thread_safety.cpp gtest_xml_null_subtree_id.cpp + gtest_xml_include_rollback.cpp + gtest_tree_destructor.cpp script_parser_test.cpp test_helper.hpp diff --git a/tests/gtest_tree_destructor.cpp b/tests/gtest_tree_destructor.cpp new file mode 100644 index 000000000..d8712360d --- /dev/null +++ b/tests/gtest_tree_destructor.cpp @@ -0,0 +1,118 @@ +/* Copyright (C) 2024 Davide Faconti - All Rights Reserved + * + * Tree::~Tree() halts the tree. Halting is allowed to throw (a TryCatch with + * catch_on_halt ticks its catch child, and a tick may throw), and a destructor + * that lets an exception escape calls std::terminate. Destroying a tree whose + * halt throws must not abort the process. + */ + +#include "behaviortree_cpp/bt_factory.h" + +#include + +using namespace BT; + +namespace +{ +// Stays RUNNING, so TryCatch is still in its try block when the tree is +// destroyed - that is the condition for catch_on_halt to tick the catch child. +class RunningAction : public StatefulActionNode +{ +public: + RunningAction(const std::string& name, const NodeConfig& config) + : StatefulActionNode(name, config) + {} + + static PortsList providedPorts() + { + return {}; + } + + NodeStatus onStart() override + { + return NodeStatus::RUNNING; + } + NodeStatus onRunning() override + { + return NodeStatus::RUNNING; + } + void onHalted() override + {} +}; + +// Throws on every tick, including the one a TryCatch does while halting. +// Stateful, because a SyncActionNode that returns RUNNING is itself an error. +class ThrowingAction : public StatefulActionNode +{ +public: + ThrowingAction(const std::string& name, const NodeConfig& config) + : StatefulActionNode(name, config) + {} + + static PortsList providedPorts() + { + return {}; + } + + NodeStatus onStart() override + { + throw std::runtime_error("thrown from tick"); + } + NodeStatus onRunning() override + { + throw std::runtime_error("thrown from tick"); + } + void onHalted() override + {} +}; + +} // namespace + +// The tree reaches RUNNING, so TryCatch takes the catch_on_halt path when the +// destructor halts it; the catch child throws and the exception must not +// escape the destructor. +TEST(TreeDestructorTest, HaltThatThrowsDoesNotTerminate) +{ + BehaviorTreeFactory factory; + factory.registerNodeType("RunningAction"); + factory.registerNodeType("ThrowingAction"); + + const std::string xml_text = R"( + + + + + + + + + )"; + + // Reaching this line is the assertion: before the fix the destructor's + // exception escaped and the process called std::terminate. + EXPECT_NO_THROW({ + auto tree = factory.createTreeFromText(xml_text); + tree.tickOnce(); + }); +} + +// A plain tree without a throwing node must still be destroyed normally, and +// halting must still happen - the fix must not skip it. +TEST(TreeDestructorTest, HaltStillRuns) +{ + BehaviorTreeFactory factory; + + const std::string xml_text = R"( + + + + + + )"; + + auto tree = factory.createTreeFromText(xml_text); + EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS); + // Scoped, so the destructor runs here and the test can observe it. + EXPECT_NO_THROW({ auto scoped = factory.createTreeFromText(xml_text); + scoped.tickOnce(); }); +} diff --git a/tests/gtest_xml_include_rollback.cpp b/tests/gtest_xml_include_rollback.cpp new file mode 100644 index 000000000..273967261 --- /dev/null +++ b/tests/gtest_xml_include_rollback.cpp @@ -0,0 +1,199 @@ +/* Copyright (C) 2024 Davide Faconti - All Rights Reserved + * + * Test for the rollback in XMLParser::loadFromFile(): a load call that is + * rejected must not leave the trees registered by the documents it pulled in + * through . + */ + +#include "behaviortree_cpp/bt_factory.h" +#include "behaviortree_cpp/xml_parsing.h" + +#include + +#include +#include + +using namespace BT; + +namespace +{ +class TempDir +{ +public: + TempDir() + { + _path = std::filesystem::temp_directory_path() / + ("bt_xmlparser_" + std::to_string(::getpid()) + "_" + + std::to_string(reinterpret_cast(this))); + std::filesystem::create_directories(_path); + } + + ~TempDir() + { + std::error_code ec; + std::filesystem::remove_all(_path, ec); + } + + std::filesystem::path write(const std::string& name, const std::string& content) const + { + const auto path = _path / name; + std::ofstream out(path); + out << content; + return path; + } + + const std::filesystem::path& path() const + { + return _path; + } + +private: + std::filesystem::path _path; +}; + +bool contains(const std::vector& names, const std::string& name) +{ + return std::find(names.begin(), names.end(), name) != names.end(); +} +} // namespace + +// The included document is valid and registers a tree; the including document +// is rejected afterwards. Before the fix only the rejected document was +// dropped, so the tree from the include stayed registered even though the +// load call as a whole failed. +TEST(XMLParserTest, RejectedIncludeKeepsNoTree) +{ + TempDir dir; + dir.write("sub.xml", R"( + + + + )"); + + const auto main_xml = dir.write("main.xml", R"( + + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + + EXPECT_THROW(parser.loadFromFile(main_xml), std::exception); + + const auto registered = parser.registeredBehaviorTrees(); + EXPECT_FALSE(contains(registered, "FromInclude")) + << "a rejected load call must not leave the included tree registered"; + EXPECT_FALSE(contains(registered, "MainTree")); +} + +// The same for loadFromText(), which shares the rollback path. +TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree) +{ + TempDir dir; + dir.write("sub.xml", R"( + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + + const std::string text = R"( + + + + + )"; + + EXPECT_THROW(parser.loadFromText(text), std::exception); + EXPECT_FALSE(contains(parser.registeredBehaviorTrees(), "FromInclude")); +} + +// A load call that succeeds must still register everything, includes included. +TEST(XMLParserTest, AcceptedIncludeKeepsItsTree) +{ + TempDir dir; + dir.write("sub.xml", R"( + + + + )"); + + const auto main_xml = dir.write("main.xml", R"( + + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + parser.loadFromFile(main_xml); + + const auto registered = parser.registeredBehaviorTrees(); + EXPECT_TRUE(contains(registered, "FromInclude")); + EXPECT_TRUE(contains(registered, "MainTree")); +} + +// A rejected load that redefined a tree must put the previous definition back: +// the new element lives in the document that is about to be erased, so leaving +// it in place would leave tree_roots pointing into freed memory. +TEST(XMLParserTest, RejectedRedefinitionRestoresPreviousTree) +{ + TempDir dir; + const auto good = dir.write("good.xml", R"( + + + + )"); + + const auto bad = dir.write("bad.xml", R"( + + + + )"); + + // Use the factory's own parser: createTree() reads that one's tree_roots, + // so this exercises the same state a user would hit. + BehaviorTreeFactory factory; + factory.registerBehaviorTreeFromFile(good); + + EXPECT_THROW(factory.registerBehaviorTreeFromFile(bad), std::exception); + + // The first definition must still be usable, not just present by name. + EXPECT_NO_THROW({ + auto tree = factory.createTree("Shared"); + EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS); + }); +} + +// Trees registered by an earlier, successful load must survive a later +// rejected one. +TEST(XMLParserTest, RollbackKeepsEarlierTrees) +{ + TempDir dir; + const auto good = dir.write("good.xml", R"( + + + + )"); + + const auto bad = dir.write("bad.xml", R"( + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + parser.loadFromFile(good); + + EXPECT_THROW(parser.loadFromFile(bad), std::exception); + + const auto registered = parser.registeredBehaviorTrees(); + EXPECT_TRUE(contains(registered, "Good")); + EXPECT_FALSE(contains(registered, "Bad")); +}