diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index e3e376c50..2d7d67d27 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -720,7 +720,20 @@ std::shared_ptr Tree::wakeUpSignal() const Tree::~Tree() { - haltTree(); + // 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 c7f0f3273..03d433b8a 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -248,21 +248,31 @@ 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. + // 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()); + 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 { loadDocImpl(doc_it->get(), add_includes); } catch(...) { - opened_documents.erase(doc_it); + 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.end()); throw; } } @@ -466,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 5d4e643c9..8a642b8d6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -61,6 +61,7 @@ set(BT_TESTS gtest_plugin_issue953.cpp gtest_blackboard_thread_safety.cpp gtest_xml_null_subtree_id.cpp + gtest_xml_include_rollback.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_try_catch.cpp b/tests/gtest_try_catch.cpp index 5ff26dbe7..5de9559ae 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,43 @@ 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). +// 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("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" }) + { + 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, 2); +} + 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 new file mode 100644 index 000000000..69ac7d814 --- /dev/null +++ b/tests/gtest_xml_include_rollback.cpp @@ -0,0 +1,116 @@ +/* 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 + +using namespace BT; +using Names = std::vector; + +namespace +{ +std::string treeXml(const std::string& body) +{ + return R"()" + body + ""; +} + +std::string tree(const std::string& tree_id, const std::string& node) +{ + 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 + +TEST(XMLParserTest, RejectedIncludeKeepsNoTree) +{ + TempSubdir dir; + const auto main_xml = writeMainWithInclude(dir, "NotARegisteredNode"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + EXPECT_THROW(parser.loadFromFile(main_xml), std::exception); + EXPECT_EQ(parser.registeredBehaviorTrees(), Names{}); +} + +TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree) +{ + TempSubdir dir; + writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess"))); + const auto text = treeXml(R"()" + tree("MainTree", "NotARegisteredNode")); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + EXPECT_THROW(parser.loadFromText(text), std::exception); + EXPECT_EQ(parser.registeredBehaviorTrees(), Names{}); +} + +TEST(XMLParserTest, AcceptedIncludeKeepsItsTree) +{ + TempSubdir dir; + const auto main_xml = writeMainWithInclude(dir, "AlwaysSuccess"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + parser.loadFromFile(main_xml); + EXPECT_EQ(parser.registeredBehaviorTrees(), (Names{ "FromInclude", "MainTree" })); +} + +// 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) +{ + 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"))); + + BehaviorTreeFactory factory; + EXPECT_THROW(factory.registerBehaviorTreeFromFile(main_xml), std::exception); + + // 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); +} + +// 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) +{ + 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; + factory.registerBehaviorTreeFromFile(dir.path / "good.xml"); + EXPECT_THROW(factory.registerBehaviorTreeFromFile(dir.path / "main.xml"), + std::exception); + + 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