From 643ffde2c43ad3db3145e3b2cbb408d2b05d7fdd Mon Sep 17 00:00:00 2001 From: Chaosheng Qu Date: Wed, 11 Nov 2026 16:31:10 +0800 Subject: [PATCH 1/5] Roll back a rejected XML load as a whole, not per document #1266 dropped a document that failed to load, but the rollback stopped at the document boundary. If main.xml includes sub.xml and then fails, sub.xml has already registered its trees and they stay behind, and sub.xml becomes the first document, so instantiateTree() reads main_tree_to_execute from it. Take a snapshot before the call and restore it on failure. That also puts back a tree this call redefined, whose element lives in the document about to be erased. Fixes #1282 --- src/xml_parsing.cpp | 12 ++ tests/CMakeLists.txt | 2 + tests/gtest_xml_include_rollback.cpp | 199 +++++++++++++++++++++++++++ 3 files changed, 213 insertions(+) create mode 100644 tests/gtest_xml_include_rollback.cpp diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index c7f0f3273..9de2db3e9 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_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")); +} From 166c716b05d4726b4275d967fe932033563160fa Mon Sep 17 00:00:00 2001 From: Chaosheng Qu Date: Wed, 11 Nov 2026 16:31:20 +0800 Subject: [PATCH 2/5] Don't let an exception escape Tree's destructor Tree::~Tree() calls haltTree(), and halting may throw: a TryCatch with catch_on_halt="true" ticks its catch child, and a tick is allowed to throw. An exception leaving a destructor calls std::terminate, so destroying such a tree aborted the process. Mark the destructor noexcept and swallow the exception. There is no caller left to report to. Fixes #1281 --- include/behaviortree_cpp/bt_factory.h | 2 +- src/bt_factory.cpp | 14 ++- tests/gtest_tree_destructor.cpp | 118 ++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 tests/gtest_tree_destructor.cpp diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index e5640df25..bbf15eba7 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/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(); }); +} From 1528a85b1868f7e6396d41a4d9d3d967a444134b Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 21:57:24 +0100 Subject: [PATCH 3/5] Apply pre-commit (clang-format) Co-Authored-By: Claude Opus 5.5 --- src/bt_factory.cpp | 3 +-- tests/gtest_tree_destructor.cpp | 6 ++++-- tests/gtest_xml_include_rollback.cpp | 7 ++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index 2f0166d9b..e66fc35f5 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -729,8 +729,7 @@ Tree::~Tree() noexcept haltTree(); } catch(...) - { - } + {} } NodeStatus Tree::tickExactlyOnce() diff --git a/tests/gtest_tree_destructor.cpp b/tests/gtest_tree_destructor.cpp index d8712360d..a081ff715 100644 --- a/tests/gtest_tree_destructor.cpp +++ b/tests/gtest_tree_destructor.cpp @@ -113,6 +113,8 @@ TEST(TreeDestructorTest, HaltStillRuns) 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(); }); + 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 index 273967261..9c34ce448 100644 --- a/tests/gtest_xml_include_rollback.cpp +++ b/tests/gtest_xml_include_rollback.cpp @@ -8,11 +8,11 @@ #include "behaviortree_cpp/bt_factory.h" #include "behaviortree_cpp/xml_parsing.h" -#include - #include #include +#include + using namespace BT; namespace @@ -102,7 +102,8 @@ TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree) XMLParser parser(factory); const std::string text = R"( - + From 60e22c9304cbfd037ca80f57b8213a535c81d82b Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 22:07:16 +0100 Subject: [PATCH 4/5] Roll back a rejected load once, at the top-level call; simplify tests - loadLastDocImpl is now only the top-level rollback point: calls loadDocImpl directly, and on failure the call drops every document it opened (erase(doc_it, end())), so a valid included document no longer stays the "first document" whose main_tree_to_execute instantiateTree() reads. The snapshot also covers subtree_models and current_path, as listed in #1282, and is restored by move. - ~Tree(): drop the redundant noexcept (destructors are implicitly noexcept) and report a throwing halt on stderr, as FinallyNode does, instead of swallowing it silently (also fixes bugprone-empty-catch). - Tests: share TempSubdir/writeFile through test_helper.hpp (no getpid, which broke MSVC); move the destructor test next to the other catch_on_halt tests using lambda nodes and assert the catch child ran; add a first-document test; make the redefinition test go through a valid include of a rejected file (the old one passed on master); drop HaltStillRuns and RollbackKeepsEarlierTrees, which asserted nothing new. Co-Authored-By: Claude Opus 5.5 --- include/behaviortree_cpp/bt_factory.h | 2 +- src/bt_factory.cpp | 16 +- src/xml_parsing.cpp | 33 ++-- tests/CMakeLists.txt | 1 - tests/gtest_duplicate_tree.cpp | 33 +--- tests/gtest_tree_destructor.cpp | 120 -------------- tests/gtest_try_catch.cpp | 32 ++++ tests/gtest_xml_include_rollback.cpp | 216 ++++++++------------------ tests/test_helper.hpp | 35 +++++ 9 files changed, 162 insertions(+), 326 deletions(-) delete mode 100644 tests/gtest_tree_destructor.cpp diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index bbf15eba7..e5640df25 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() noexcept; + ~Tree(); /// 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 e66fc35f5..2d7d67d27 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -718,18 +718,22 @@ std::shared_ptr Tree::wakeUpSignal() const return wake_up_; } -Tree::~Tree() noexcept +Tree::~Tree() { - // 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. + // Halting may throw (e.g. TryCatch catch_on_halt ticks its catch child), and + // an exception escaping a destructor calls std::terminate (issue #1281). try { haltTree(); } + catch(const std::exception& ex) + { + std::cerr << "[Tree::~Tree]: haltTree() threw: " << ex.what() << std::endl; + } catch(...) - {} + { + std::cerr << "[Tree::~Tree]: haltTree() threw an unknown exception" << std::endl; + } } NodeStatus Tree::tickExactlyOnce() diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 9de2db3e9..03d433b8a 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -248,22 +248,18 @@ struct XMLParser::PImpl void loadDocImpl(XMLDocument* doc, bool add_includes); - // Load the document just appended to opened_documents. A document that is - // rejected (XML error, invalid tree) registers no tree, so forget it again: - // 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. + // Load the document just appended to opened_documents, all or nothing. + // Through , one call loads several documents: on failure restore + // the state from before the call and drop every document it opened, so none + // stays the "first opened document" instantiateTree() reads + // main_tree_to_execute from (issues #1266, #1282). 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; + auto roots_before = tree_roots; + auto sources_before = tree_sources; + auto models_before = subtree_models; + auto path_before = current_path; const int suffix_before = suffix_count; try { @@ -271,10 +267,12 @@ struct XMLParser::PImpl } catch(...) { - tree_roots = roots_before; - tree_sources = sources_before; + tree_roots = std::move(roots_before); + tree_sources = std::move(sources_before); + subtree_models = std::move(models_before); + current_path = std::move(path_before); suffix_count = suffix_before; - opened_documents.erase(doc_it); + opened_documents.erase(doc_it, opened_documents.end()); throw; } } @@ -478,7 +476,8 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) include_stack.push_back(std::move(include_key)); next_doc->LoadFile(file_path.string().c_str()); - loadLastDocImpl(add_includes); + // no rollback here: the top-level load call undoes the whole load + loadDocImpl(next_doc, add_includes); include_stack.pop_back(); // reset current path to the previous value diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 31c9aa9d0..8a642b8d6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -62,7 +62,6 @@ set(BT_TESTS 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_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp index ea3b99ec4..94f7df2b6 100644 --- a/tests/gtest_duplicate_tree.cpp +++ b/tests/gtest_duplicate_tree.cpp @@ -1,10 +1,10 @@ /* Copyright (C) 2026 - duplicate BehaviorTree ID registration tests */ +#include "test_helper.hpp" + #include "behaviortree_cpp/bt_factory.h" -#include #include -#include #include #include @@ -14,35 +14,6 @@ using namespace BT; namespace { -class TempSubdir -{ -public: - TempSubdir() - { - const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count(); - path = std::filesystem::temp_directory_path() / - ("bt_duplicate_tree_" + std::to_string(stamp)); - std::filesystem::create_directories(path); - } - - ~TempSubdir() - { - std::error_code ec; - std::filesystem::remove_all(path, ec); - } - - std::filesystem::path path; - - TempSubdir(const TempSubdir&) = delete; - TempSubdir& operator=(const TempSubdir&) = delete; -}; - -void writeFile(const std::filesystem::path& p, const std::string& content) -{ - std::ofstream out(p.string()); - out << content; -} - std::string minimalTreeXml(const std::string& tree_id, const std::string& node = "AlwaysS" "uccess") { diff --git a/tests/gtest_tree_destructor.cpp b/tests/gtest_tree_destructor.cpp deleted file mode 100644 index a081ff715..000000000 --- a/tests/gtest_tree_destructor.cpp +++ /dev/null @@ -1,120 +0,0 @@ -/* 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_try_catch.cpp b/tests/gtest_try_catch.cpp index 5ff26dbe7..0b5d9f0f8 100644 --- a/tests/gtest_try_catch.cpp +++ b/tests/gtest_try_catch.cpp @@ -14,6 +14,8 @@ #include "behaviortree_cpp/bt_factory.h" +#include + #include using BT::NodeStatus; @@ -341,6 +343,36 @@ TEST_F(TryCatchTest, CatchOnHalt_Enabled) ASSERT_EQ(catch_count, 1); // Catch executed on halt } +// Destroying a RUNNING tree halts it; a catch child that throws during that +// halt must not escape Tree::~Tree() and call std::terminate (issue #1281). +TEST_F(TryCatchTest, CatchOnHalt_ThrowInDestructorDoesNotTerminate) +{ + factory.registerSimpleCondition("AlwaysRunning", + [](BT::TreeNode&) { return NodeStatus::RUNNING; }); + int catch_ticks = 0; + factory.registerSimpleAction("ThrowingCatch", + [&catch_ticks](BT::TreeNode&) -> NodeStatus { + catch_ticks++; + throw std::runtime_error("thrown while halting"); + }); + + const std::string xml_text = R"( + + + + + + + + )"; + + { + auto tree = factory.createTreeFromText(xml_text); + EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + } + EXPECT_EQ(catch_ticks, 1); +} + TEST_F(TryCatchTest, CatchOnHalt_NotTriggeredWhenAlreadyInCatch) { int catch_ticks = 0; diff --git a/tests/gtest_xml_include_rollback.cpp b/tests/gtest_xml_include_rollback.cpp index 9c34ce448..69ac7d814 100644 --- a/tests/gtest_xml_include_rollback.cpp +++ b/tests/gtest_xml_include_rollback.cpp @@ -1,200 +1,116 @@ -/* 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 . - */ +/* A load call rejected after an must leave the parser as it was + * before the call (issue #1282). */ + +#include "test_helper.hpp" #include "behaviortree_cpp/bt_factory.h" #include "behaviortree_cpp/xml_parsing.h" -#include -#include +#include +#include +#include #include using namespace BT; +using Names = std::vector; namespace { -class TempDir +std::string treeXml(const std::string& body) { -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 R"()" + body + ""; +} + +std::string tree(const std::string& tree_id, const std::string& node) { - return std::find(names.begin(), names.end(), name) != names.end(); + return R"(<)" + node + "/>"; +} + +// main.xml includes a valid sub.xml, then defines MainTree with `main_node` +std::filesystem::path writeMainWithInclude(const TempSubdir& dir, + const std::string& main_node) +{ + writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess"))); + const auto main_xml = dir.path / "main.xml"; + writeFile(main_xml, + treeXml(R"()" + tree("MainTree", main_node))); + return main_xml; } } // 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"( - - - - - )"); + TempSubdir dir; + const auto main_xml = writeMainWithInclude(dir, "NotARegisteredNode"); 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")); + EXPECT_EQ(parser.registeredBehaviorTrees(), Names{}); } -// The same for loadFromText(), which shares the rollback path. TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree) { - TempDir dir; - dir.write("sub.xml", R"( - - - - )"); + TempSubdir dir; + writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess"))); + const auto text = treeXml(R"()" + tree("MainTree", "NotARegisteredNode")); BehaviorTreeFactory factory; XMLParser parser(factory); - - const std::string text = R"( - - - - - )"; - EXPECT_THROW(parser.loadFromText(text), std::exception); - EXPECT_FALSE(contains(parser.registeredBehaviorTrees(), "FromInclude")); + EXPECT_EQ(parser.registeredBehaviorTrees(), Names{}); } -// 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"( - - - - - )"); + TempSubdir dir; + const auto main_xml = writeMainWithInclude(dir, "AlwaysSuccess"); BehaviorTreeFactory factory; XMLParser parser(factory); parser.loadFromFile(main_xml); - - const auto registered = parser.registeredBehaviorTrees(); - EXPECT_TRUE(contains(registered, "FromInclude")); - EXPECT_TRUE(contains(registered, "MainTree")); + EXPECT_EQ(parser.registeredBehaviorTrees(), (Names{ "FromInclude", "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) +// The rejected load's documents must all be dropped: a leftover included +// document would become the first one, and its main_tree_to_execute would be +// picked for a later file that does not name its main tree. +TEST(XMLParserTest, RejectedIncludeIsNotTheFirstDocument) { - 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); + TempSubdir dir; + writeFile(dir.path / "sub.xml", + R"()" + + tree("FromInclude", "AlwaysSuccess") + ""); + const auto main_xml = dir.path / "main.xml"; + writeFile(main_xml, treeXml(R"()" + + tree("MainTree", "NotARegisteredNode"))); - EXPECT_THROW(factory.registerBehaviorTreeFromFile(bad), std::exception); + BehaviorTreeFactory factory; + EXPECT_THROW(factory.registerBehaviorTreeFromFile(main_xml), 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); - }); + // two trees and no main_tree_to_execute: ambiguous, must be rejected + const auto ambiguous = + treeXml(tree("A", "AlwaysSuccess") + tree("FromInclude", "AlwaysFailure")); + EXPECT_THROW(std::ignore = factory.createTreeFromText(ambiguous), RuntimeError); } -// Trees registered by an earlier, successful load must survive a later -// rejected one. -TEST(XMLParserTest, RollbackKeepsEarlierTrees) +// A tree redefined by a valid include of a rejected file must get its +// previous definition back: the redefining document is erased. +TEST(XMLParserTest, RejectedRedefinitionRestoresPreviousTree) { - TempDir dir; - const auto good = dir.write("good.xml", R"( - - - - )"); - - const auto bad = dir.write("bad.xml", R"( - - - - )"); + TempSubdir dir; + writeFile(dir.path / "good.xml", treeXml(tree("Shared", "AlwaysSuccess"))); + writeFile(dir.path / "redefine.xml", treeXml(tree("Shared", "AlwaysFailure"))); + writeFile(dir.path / "main.xml", treeXml(R"()" + + tree("MainTree", "NotARegisteredNode"))); BehaviorTreeFactory factory; - XMLParser parser(factory); - parser.loadFromFile(good); - - EXPECT_THROW(parser.loadFromFile(bad), std::exception); + factory.registerBehaviorTreeFromFile(dir.path / "good.xml"); + EXPECT_THROW(factory.registerBehaviorTreeFromFile(dir.path / "main.xml"), + std::exception); - const auto registered = parser.registeredBehaviorTrees(); - EXPECT_TRUE(contains(registered, "Good")); - EXPECT_FALSE(contains(registered, "Bad")); + auto tree_instance = factory.createTree("Shared"); + EXPECT_EQ(tree_instance.tickOnce(), NodeStatus::SUCCESS); } diff --git a/tests/test_helper.hpp b/tests/test_helper.hpp index c03b80fab..aa98fd570 100644 --- a/tests/test_helper.hpp +++ b/tests/test_helper.hpp @@ -3,8 +3,11 @@ #include "behaviortree_cpp/bt_factory.h" +#include #include #include +#include +#include #include #if !defined(_WIN32) @@ -85,4 +88,36 @@ inline void RegisterTestTick(BT::BehaviorTreeFactory& factory, } } +/// A fresh directory under the system temp dir, removed with its content at +/// the end of the scope. +class TempSubdir +{ +public: + TempSubdir() + { + const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count(); + path = std::filesystem::temp_directory_path() / ("bt_test_" + std::to_string(stamp)); + std::filesystem::create_directories(path); + } + + ~TempSubdir() + { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + + std::filesystem::path path; + + TempSubdir(const TempSubdir&) = delete; + TempSubdir& operator=(const TempSubdir&) = delete; + TempSubdir(TempSubdir&&) = delete; + TempSubdir& operator=(TempSubdir&&) = delete; +}; + +inline void writeFile(const std::filesystem::path& p, const std::string& content) +{ + std::ofstream out(p.string()); + out << content; +} + #endif // TEST_HELPER_HPP From 1fb063bd94199d03ba5e58374286b65b206756ed Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 12 Nov 2026 22:24:27 +0100 Subject: [PATCH 5/5] Cover a non-std exception thrown while halting in Tree::~Tree() Co-Authored-By: Claude Opus 5.5 --- tests/gtest_try_catch.cpp | 43 +++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/tests/gtest_try_catch.cpp b/tests/gtest_try_catch.cpp index 0b5d9f0f8..5de9559ae 100644 --- a/tests/gtest_try_catch.cpp +++ b/tests/gtest_try_catch.cpp @@ -345,32 +345,39 @@ TEST_F(TryCatchTest, CatchOnHalt_Enabled) // Destroying a RUNNING tree halts it; a catch child that throws during that // halt must not escape Tree::~Tree() and call std::terminate (issue #1281). +// Covers both a std::exception and any other thrown type. TEST_F(TryCatchTest, CatchOnHalt_ThrowInDestructorDoesNotTerminate) { factory.registerSimpleCondition("AlwaysRunning", [](BT::TreeNode&) { return NodeStatus::RUNNING; }); int catch_ticks = 0; - factory.registerSimpleAction("ThrowingCatch", - [&catch_ticks](BT::TreeNode&) -> NodeStatus { - catch_ticks++; - throw std::runtime_error("thrown while halting"); - }); - - const std::string xml_text = R"( - - - - - - - - )"; + factory.registerSimpleAction("ThrowStd", [&catch_ticks](BT::TreeNode&) -> NodeStatus { + catch_ticks++; + throw std::runtime_error("thrown while halting"); + }); + factory.registerSimpleAction("ThrowInt", [&catch_ticks](BT::TreeNode&) -> NodeStatus { + catch_ticks++; + throw 42; + }); + for(const std::string catch_node : { "ThrowStd", "ThrowInt" }) { - auto tree = factory.createTreeFromText(xml_text); - EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + const std::string xml_text = R"( + + + + + <)" + catch_node + R"(/> + + + )"; + + { + auto tree = factory.createTreeFromText(xml_text); + EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + } } - EXPECT_EQ(catch_ticks, 1); + EXPECT_EQ(catch_ticks, 2); } TEST_F(TryCatchTest, CatchOnHalt_NotTriggeredWhenAlreadyInCatch)