Roll back a rejected XML load as a whole, and keep Tree's destructor from throwing

include/behaviortree_cpp/bt_factory.h

@@ -154,7 +154,7 @@ class Tree
    */
   [[nodiscard]] std::shared_ptr<WakeUpSignal> wakeUpSignal() const;
 
-  ~Tree();
+  ~Tree() noexcept;
 
   /// Tick the root of the tree once, even if a node invoked
   /// emitWakeUpSignal()

src/bt_factory.cpp

@@ -718,9 +718,19 @@ std::shared_ptr<WakeUpSignal> 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()

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: <include> 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;
     }

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

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 <gtest/gtest.h>
+
+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>("RunningAction");
+  factory.registerNodeType<ThrowingAction>("ThrowingAction");
+
+  const std::string xml_text = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="MainTree">
+      <TryCatch catch_on_halt="true">
+        <RunningAction/>
+        <ThrowingAction/>
+      </TryCatch>
+    </BehaviorTree>
+  </root>
+  )";
+
+  // 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"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="MainTree">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+  </root>
+  )";
+
+  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(); });
+}

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>.
+ */
+
+#include "behaviortree_cpp/bt_factory.h"
+#include "behaviortree_cpp/xml_parsing.h"
+
+#include <gtest/gtest.h>
+
+#include <filesystem>
+#include <fstream>
+
+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<uintptr_t>(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<std::string>& 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"(<root BTCPP_format="4">
+    <BehaviorTree ID="FromInclude">
+      <AlwaysSuccess />
+    </BehaviorTree>
+  </root>)");
+
+  const auto main_xml = dir.write("main.xml", R"(<root BTCPP_format="4">
+    <include path="sub.xml"/>
+    <BehaviorTree ID="MainTree">
+      <NotARegisteredNode />
+    </BehaviorTree>
+  </root>)");
+
+  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"(<root BTCPP_format="4">
+    <BehaviorTree ID="FromInclude">
+      <AlwaysSuccess />
+    </BehaviorTree>
+  </root>)");
+
+  BehaviorTreeFactory factory;
+  XMLParser parser(factory);
+
+  const std::string text = R"(<root BTCPP_format="4">
+    <include path=")" + dir.path().string() + R"(/sub.xml"/>
+    <BehaviorTree ID="MainTree">
+      <NotARegisteredNode />
+    </BehaviorTree>
+  </root>)";
+
+  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"(<root BTCPP_format="4">
+    <BehaviorTree ID="FromInclude">
+      <AlwaysSuccess />
+    </BehaviorTree>
+  </root>)");
+
+  const auto main_xml = dir.write("main.xml", R"(<root BTCPP_format="4">
+    <include path="sub.xml"/>
+    <BehaviorTree ID="MainTree">
+      <AlwaysSuccess />
+    </BehaviorTree>
+  </root>)");
+
+  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"(<root BTCPP_format="4">
+    <BehaviorTree ID="Shared">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+  </root>)");
+
+  const auto bad = dir.write("bad.xml", R"(<root BTCPP_format="4">
+    <BehaviorTree ID="Shared">
+      <NotARegisteredNode />
+    </BehaviorTree>
+  </root>)");
+
+  // 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"(<root BTCPP_format="4">
+    <BehaviorTree ID="Good">
+      <AlwaysSuccess />
+    </BehaviorTree>
+  </root>)");
+
+  const auto bad = dir.write("bad.xml", R"(<root BTCPP_format="4">
+    <BehaviorTree ID="Bad">
+      <NotARegisteredNode />
+    </BehaviorTree>
+  </root>)");
+
+  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"));
+}