Skip to content

Commit 0ffae1c

Browse files
Quchaoshengfacontidavideclaude
authored
Roll back a rejected XML load as a whole, and keep Tree's destructor from throwing (#1289)
* 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 * 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 * Apply pre-commit (clang-format) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Roll back a rejected load once, at the top-level call; simplify tests - loadLastDocImpl is now only the top-level rollback point: <include> 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 <noreply@anthropic.com> * Cover a non-std exception thrown while halting in Tree::~Tree() Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Davide Faconti <davide.faconti@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 9174257 commit 0ffae1c

7 files changed

Lines changed: 225 additions & 39 deletions

File tree

‎src/bt_factory.cpp‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,20 @@ std::shared_ptr<WakeUpSignal> Tree::wakeUpSignal() const
720720

721721
Tree::~Tree()
722722
{
723-
haltTree();
723+
// Halting may throw (e.g. TryCatch catch_on_halt ticks its catch child), and
724+
// an exception escaping a destructor calls std::terminate (issue #1281).
725+
try
726+
{
727+
haltTree();
728+
}
729+
catch(const std::exception& ex)
730+
{
731+
std::cerr << "[Tree::~Tree]: haltTree() threw: " << ex.what() << std::endl;
732+
}
733+
catch(...)
734+
{
735+
std::cerr << "[Tree::~Tree]: haltTree() threw an unknown exception" << std::endl;
736+
}
724737
}
725738

726739
NodeStatus Tree::tickExactlyOnce()

‎src/xml_parsing.cpp‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -248,21 +248,31 @@ struct XMLParser::PImpl
248248

249249
void loadDocImpl(XMLDocument* doc, bool add_includes);
250250

251-
// Load the document just appended to opened_documents. A document that is
252-
// rejected (XML error, invalid tree) registers no tree, so forget it again:
253-
// it would otherwise remain the "first opened document" that
254-
// instantiateTree() reads main_tree_to_execute from, and a document that
255-
// did not parse has no root element at all.
251+
// Load the document just appended to opened_documents, all or nothing.
252+
// Through <include>, one call loads several documents: on failure restore
253+
// the state from before the call and drop every document it opened, so none
254+
// stays the "first opened document" instantiateTree() reads
255+
// main_tree_to_execute from (issues #1266, #1282).
256256
void loadLastDocImpl(bool add_includes)
257257
{
258258
const auto doc_it = std::prev(opened_documents.end());
259+
auto roots_before = tree_roots;
260+
auto sources_before = tree_sources;
261+
auto models_before = subtree_models;
262+
auto path_before = current_path;
263+
const int suffix_before = suffix_count;
259264
try
260265
{
261266
loadDocImpl(doc_it->get(), add_includes);
262267
}
263268
catch(...)
264269
{
265-
opened_documents.erase(doc_it);
270+
tree_roots = std::move(roots_before);
271+
tree_sources = std::move(sources_before);
272+
subtree_models = std::move(models_before);
273+
current_path = std::move(path_before);
274+
suffix_count = suffix_before;
275+
opened_documents.erase(doc_it, opened_documents.end());
266276
throw;
267277
}
268278
}
@@ -466,7 +476,8 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes)
466476

467477
include_stack.push_back(std::move(include_key));
468478
next_doc->LoadFile(file_path.string().c_str());
469-
loadLastDocImpl(add_includes);
479+
// no rollback here: the top-level load call undoes the whole load
480+
loadDocImpl(next_doc, add_includes);
470481
include_stack.pop_back();
471482

472483
// reset current path to the previous value

‎tests/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ set(BT_TESTS
6161
gtest_plugin_issue953.cpp
6262
gtest_blackboard_thread_safety.cpp
6363
gtest_xml_null_subtree_id.cpp
64+
gtest_xml_include_rollback.cpp
6465

6566
script_parser_test.cpp
6667
test_helper.hpp

‎tests/gtest_duplicate_tree.cpp‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
/* Copyright (C) 2026 - duplicate BehaviorTree ID registration tests */
22

3+
#include "test_helper.hpp"
4+
35
#include "behaviortree_cpp/bt_factory.h"
46

5-
#include <chrono>
67
#include <filesystem>
7-
#include <fstream>
88
#include <string>
99
#include <tuple>
1010

@@ -14,35 +14,6 @@ using namespace BT;
1414

1515
namespace
1616
{
17-
class TempSubdir
18-
{
19-
public:
20-
TempSubdir()
21-
{
22-
const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count();
23-
path = std::filesystem::temp_directory_path() /
24-
("bt_duplicate_tree_" + std::to_string(stamp));
25-
std::filesystem::create_directories(path);
26-
}
27-
28-
~TempSubdir()
29-
{
30-
std::error_code ec;
31-
std::filesystem::remove_all(path, ec);
32-
}
33-
34-
std::filesystem::path path;
35-
36-
TempSubdir(const TempSubdir&) = delete;
37-
TempSubdir& operator=(const TempSubdir&) = delete;
38-
};
39-
40-
void writeFile(const std::filesystem::path& p, const std::string& content)
41-
{
42-
std::ofstream out(p.string());
43-
out << content;
44-
}
45-
4617
std::string minimalTreeXml(const std::string& tree_id, const std::string& node = "AlwaysS"
4718
"uccess")
4819
{

‎tests/gtest_try_catch.cpp‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
#include "behaviortree_cpp/bt_factory.h"
1616

17+
#include <stdexcept>
18+
1719
#include <gtest/gtest.h>
1820

1921
using BT::NodeStatus;
@@ -341,6 +343,43 @@ TEST_F(TryCatchTest, CatchOnHalt_Enabled)
341343
ASSERT_EQ(catch_count, 1); // Catch executed on halt
342344
}
343345

346+
// Destroying a RUNNING tree halts it; a catch child that throws during that
347+
// halt must not escape Tree::~Tree() and call std::terminate (issue #1281).
348+
// Covers both a std::exception and any other thrown type.
349+
TEST_F(TryCatchTest, CatchOnHalt_ThrowInDestructorDoesNotTerminate)
350+
{
351+
factory.registerSimpleCondition("AlwaysRunning",
352+
[](BT::TreeNode&) { return NodeStatus::RUNNING; });
353+
int catch_ticks = 0;
354+
factory.registerSimpleAction("ThrowStd", [&catch_ticks](BT::TreeNode&) -> NodeStatus {
355+
catch_ticks++;
356+
throw std::runtime_error("thrown while halting");
357+
});
358+
factory.registerSimpleAction("ThrowInt", [&catch_ticks](BT::TreeNode&) -> NodeStatus {
359+
catch_ticks++;
360+
throw 42;
361+
});
362+
363+
for(const std::string catch_node : { "ThrowStd", "ThrowInt" })
364+
{
365+
const std::string xml_text = R"(
366+
<root BTCPP_format="4">
367+
<BehaviorTree>
368+
<TryCatch catch_on_halt="true">
369+
<AlwaysRunning/>
370+
<)" + catch_node + R"(/>
371+
</TryCatch>
372+
</BehaviorTree>
373+
</root>)";
374+
375+
{
376+
auto tree = factory.createTreeFromText(xml_text);
377+
EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING);
378+
}
379+
}
380+
EXPECT_EQ(catch_ticks, 2);
381+
}
382+
344383
TEST_F(TryCatchTest, CatchOnHalt_NotTriggeredWhenAlreadyInCatch)
345384
{
346385
int catch_ticks = 0;
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/* A load call rejected after an <include> must leave the parser as it was
2+
* before the call (issue #1282). */
3+
4+
#include "test_helper.hpp"
5+
6+
#include "behaviortree_cpp/bt_factory.h"
7+
#include "behaviortree_cpp/xml_parsing.h"
8+
9+
#include <string>
10+
#include <tuple>
11+
#include <vector>
12+
13+
#include <gtest/gtest.h>
14+
15+
using namespace BT;
16+
using Names = std::vector<std::string>;
17+
18+
namespace
19+
{
20+
std::string treeXml(const std::string& body)
21+
{
22+
return R"(<root BTCPP_format="4">)" + body + "</root>";
23+
}
24+
25+
std::string tree(const std::string& tree_id, const std::string& node)
26+
{
27+
return R"(<BehaviorTree ID=")" + tree_id + R"("><)" + node + "/></BehaviorTree>";
28+
}
29+
30+
// main.xml includes a valid sub.xml, then defines MainTree with `main_node`
31+
std::filesystem::path writeMainWithInclude(const TempSubdir& dir,
32+
const std::string& main_node)
33+
{
34+
writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess")));
35+
const auto main_xml = dir.path / "main.xml";
36+
writeFile(main_xml,
37+
treeXml(R"(<include path="sub.xml"/>)" + tree("MainTree", main_node)));
38+
return main_xml;
39+
}
40+
} // namespace
41+
42+
TEST(XMLParserTest, RejectedIncludeKeepsNoTree)
43+
{
44+
TempSubdir dir;
45+
const auto main_xml = writeMainWithInclude(dir, "NotARegisteredNode");
46+
47+
BehaviorTreeFactory factory;
48+
XMLParser parser(factory);
49+
EXPECT_THROW(parser.loadFromFile(main_xml), std::exception);
50+
EXPECT_EQ(parser.registeredBehaviorTrees(), Names{});
51+
}
52+
53+
TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree)
54+
{
55+
TempSubdir dir;
56+
writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess")));
57+
const auto text = treeXml(R"(<include path=")" + (dir.path / "sub.xml").string() +
58+
R"("/>)" + tree("MainTree", "NotARegisteredNode"));
59+
60+
BehaviorTreeFactory factory;
61+
XMLParser parser(factory);
62+
EXPECT_THROW(parser.loadFromText(text), std::exception);
63+
EXPECT_EQ(parser.registeredBehaviorTrees(), Names{});
64+
}
65+
66+
TEST(XMLParserTest, AcceptedIncludeKeepsItsTree)
67+
{
68+
TempSubdir dir;
69+
const auto main_xml = writeMainWithInclude(dir, "AlwaysSuccess");
70+
71+
BehaviorTreeFactory factory;
72+
XMLParser parser(factory);
73+
parser.loadFromFile(main_xml);
74+
EXPECT_EQ(parser.registeredBehaviorTrees(), (Names{ "FromInclude", "MainTree" }));
75+
}
76+
77+
// The rejected load's documents must all be dropped: a leftover included
78+
// document would become the first one, and its main_tree_to_execute would be
79+
// picked for a later file that does not name its main tree.
80+
TEST(XMLParserTest, RejectedIncludeIsNotTheFirstDocument)
81+
{
82+
TempSubdir dir;
83+
writeFile(dir.path / "sub.xml",
84+
R"(<root BTCPP_format="4" main_tree_to_execute="FromInclude">)" +
85+
tree("FromInclude", "AlwaysSuccess") + "</root>");
86+
const auto main_xml = dir.path / "main.xml";
87+
writeFile(main_xml, treeXml(R"(<include path="sub.xml"/>)" +
88+
tree("MainTree", "NotARegisteredNode")));
89+
90+
BehaviorTreeFactory factory;
91+
EXPECT_THROW(factory.registerBehaviorTreeFromFile(main_xml), std::exception);
92+
93+
// two trees and no main_tree_to_execute: ambiguous, must be rejected
94+
const auto ambiguous =
95+
treeXml(tree("A", "AlwaysSuccess") + tree("FromInclude", "AlwaysFailure"));
96+
EXPECT_THROW(std::ignore = factory.createTreeFromText(ambiguous), RuntimeError);
97+
}
98+
99+
// A tree redefined by a valid include of a rejected file must get its
100+
// previous definition back: the redefining document is erased.
101+
TEST(XMLParserTest, RejectedRedefinitionRestoresPreviousTree)
102+
{
103+
TempSubdir dir;
104+
writeFile(dir.path / "good.xml", treeXml(tree("Shared", "AlwaysSuccess")));
105+
writeFile(dir.path / "redefine.xml", treeXml(tree("Shared", "AlwaysFailure")));
106+
writeFile(dir.path / "main.xml", treeXml(R"(<include path="redefine.xml"/>)" +
107+
tree("MainTree", "NotARegisteredNode")));
108+
109+
BehaviorTreeFactory factory;
110+
factory.registerBehaviorTreeFromFile(dir.path / "good.xml");
111+
EXPECT_THROW(factory.registerBehaviorTreeFromFile(dir.path / "main.xml"),
112+
std::exception);
113+
114+
auto tree_instance = factory.createTree("Shared");
115+
EXPECT_EQ(tree_instance.tickOnce(), NodeStatus::SUCCESS);
116+
}

‎tests/test_helper.hpp‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@
33

44
#include "behaviortree_cpp/bt_factory.h"
55

6+
#include <chrono>
67
#include <cstdio>
78
#include <cstdlib>
9+
#include <filesystem>
10+
#include <fstream>
811
#include <string>
912

1013
#if !defined(_WIN32)
@@ -85,4 +88,36 @@ inline void RegisterTestTick(BT::BehaviorTreeFactory& factory,
8588
}
8689
}
8790

91+
/// A fresh directory under the system temp dir, removed with its content at
92+
/// the end of the scope.
93+
class TempSubdir
94+
{
95+
public:
96+
TempSubdir()
97+
{
98+
const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count();
99+
path = std::filesystem::temp_directory_path() / ("bt_test_" + std::to_string(stamp));
100+
std::filesystem::create_directories(path);
101+
}
102+
103+
~TempSubdir()
104+
{
105+
std::error_code ec;
106+
std::filesystem::remove_all(path, ec);
107+
}
108+
109+
std::filesystem::path path;
110+
111+
TempSubdir(const TempSubdir&) = delete;
112+
TempSubdir& operator=(const TempSubdir&) = delete;
113+
TempSubdir(TempSubdir&&) = delete;
114+
TempSubdir& operator=(TempSubdir&&) = delete;
115+
};
116+
117+
inline void writeFile(const std::filesystem::path& p, const std::string& content)
118+
{
119+
std::ofstream out(p.string());
120+
out << content;
121+
}
122+
88123
#endif // TEST_HELPER_HPP

0 commit comments

Comments
 (0)