From 45f76700b85ffa132db38cb86280c25e20178e87 Mon Sep 17 00:00:00 2001 From: alvintps Date: Fri, 16 Jan 2026 22:58:12 +0800 Subject: [PATCH 1/6] Fixed possible infinite loop in Groot2Publisher when destructor is called --- src/loggers/groot2_publisher.cpp | 3 +-- tests/CMakeLists.txt | 1 + tests/gtest_groot2_publisher.cpp | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 tests/gtest_groot2_publisher.cpp diff --git a/src/loggers/groot2_publisher.cpp b/src/loggers/groot2_publisher.cpp index 1a9240191..bf4e617e4 100644 --- a/src/loggers/groot2_publisher.cpp +++ b/src/loggers/groot2_publisher.cpp @@ -81,7 +81,7 @@ struct Groot2Publisher::PImpl std::string tree_xml; - std::atomic_bool active_server = false; + std::atomic_bool active_server = true; std::thread server_thread; std::mutex status_mutex; @@ -241,7 +241,6 @@ void Groot2Publisher::serverLoop() { auto const serialized_uuid = CreateRandomUUID(); - _p->active_server = true; auto& socket = _p->server; auto sendErrorReply = [&socket](const std::string& msg) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6367ef4c3..f598badcf 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,7 @@ set(BT_TESTS gtest_enums.cpp gtest_factory.cpp gtest_fallback.cpp + gtest_groot2_publisher.cpp gtest_parallel.cpp gtest_preconditions.cpp gtest_ports.cpp diff --git a/tests/gtest_groot2_publisher.cpp b/tests/gtest_groot2_publisher.cpp new file mode 100644 index 000000000..590e2493e --- /dev/null +++ b/tests/gtest_groot2_publisher.cpp @@ -0,0 +1,44 @@ +#include +#include + +#include +#include +#include + +using namespace std::chrono_literals; + +namespace +{ +static const char* xml_text = R"( + + + + + +)"; + +void throwRuntimeError() +{ + BT::BehaviorTreeFactory factory; + factory.registerSimpleAction("ThrowRuntimeError", [](BT::TreeNode&) { + throw BT::RuntimeError("Oops!"); + return BT::NodeStatus::FAILURE; + }); + + auto tree = factory.createTreeFromText(xml_text); + BT::Groot2Publisher publisher(tree); + EXPECT_THROW(tree.tickOnce(), BT::RuntimeError); +} +} // namespace + +TEST(Groot2PublisherTest, EnsureNoInfiniteLoopOnThrow) +{ + auto fut = std::async(std::launch::async, throwRuntimeError); + auto status = fut.wait_for(1s); + EXPECT_EQ(status, std::future_status::ready); + + if(status != std::future_status::ready) + { + std::exit(1); // Force exit to avoid destructor blocking + } +} From 36d3a750fb872ce57389e6780b35a40043beb8bb Mon Sep 17 00:00:00 2001 From: alvintps Date: Sun, 18 Jan 2026 14:15:30 +0800 Subject: [PATCH 2/6] Made heartbeat read/writes an atomic operation in Groot2Publisher --- src/loggers/groot2_publisher.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/loggers/groot2_publisher.cpp b/src/loggers/groot2_publisher.cpp index bf4e617e4..1880818c0 100644 --- a/src/loggers/groot2_publisher.cpp +++ b/src/loggers/groot2_publisher.cpp @@ -98,7 +98,7 @@ struct Groot2Publisher::PImpl std::unordered_map pre_hooks; std::unordered_map post_hooks; - std::chrono::system_clock::time_point last_heartbeat; + std::atomic last_heartbeat; std::chrono::milliseconds max_heartbeat_delay = std::chrono::milliseconds(5000); std::atomic_bool recording = false; @@ -251,7 +251,7 @@ void Groot2Publisher::serverLoop() }; // initialize _p->last_heartbeat - _p->last_heartbeat = std::chrono::system_clock::now(); + _p->last_heartbeat = std::chrono::steady_clock::now().time_since_epoch().count(); while(_p->active_server) { @@ -261,7 +261,7 @@ void Groot2Publisher::serverLoop() continue; } // this heartbeat will help establishing if Groot is connected or not - _p->last_heartbeat = std::chrono::system_clock::now(); + _p->last_heartbeat = std::chrono::steady_clock::now().time_since_epoch().count(); std::string const request_str = requestMsg[0].to_string(); if(request_str.size() != Monitor::RequestHeader::size()) @@ -511,10 +511,11 @@ void Groot2Publisher::heartbeatLoop() { std::this_thread::sleep_for(std::chrono::milliseconds(10)); - auto now = std::chrono::system_clock::now(); + auto now = std::chrono::steady_clock::now(); const bool prev_heartbeat = has_heartbeat; - has_heartbeat = (now - _p->last_heartbeat < _p->max_heartbeat_delay); + has_heartbeat = ((now.time_since_epoch().count() - _p->last_heartbeat) < + _p->max_heartbeat_delay.count()); // if we loose or gain heartbeat, disable/enable all breakpoints if(has_heartbeat != prev_heartbeat) From 3fb427cdd5a73825ff2fb78f2a1ffe03298ff823 Mon Sep 17 00:00:00 2001 From: alvintps Date: Sun, 18 Jan 2026 14:16:50 +0800 Subject: [PATCH 3/6] Cleanup gtest_groot2_publisher.cpp --- tests/gtest_groot2_publisher.cpp | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/gtest_groot2_publisher.cpp b/tests/gtest_groot2_publisher.cpp index 590e2493e..41a3d1432 100644 --- a/tests/gtest_groot2_publisher.cpp +++ b/tests/gtest_groot2_publisher.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -20,25 +21,28 @@ static const char* xml_text = R"( void throwRuntimeError() { BT::BehaviorTreeFactory factory; - factory.registerSimpleAction("ThrowRuntimeError", [](BT::TreeNode&) { + factory.registerSimpleAction("ThrowRuntimeError", [](BT::TreeNode&) -> BT::NodeStatus { throw BT::RuntimeError("Oops!"); - return BT::NodeStatus::FAILURE; }); auto tree = factory.createTreeFromText(xml_text); BT::Groot2Publisher publisher(tree); - EXPECT_THROW(tree.tickOnce(), BT::RuntimeError); + EXPECT_THROW(tree.tickExactlyOnce(), BT::RuntimeError); } } // namespace TEST(Groot2PublisherTest, EnsureNoInfiniteLoopOnThrow) { - auto fut = std::async(std::launch::async, throwRuntimeError); - auto status = fut.wait_for(1s); - EXPECT_EQ(status, std::future_status::ready); - - if(status != std::future_status::ready) - { - std::exit(1); // Force exit to avoid destructor blocking - } + EXPECT_EXIT( + { + auto fut = std::async(std::launch::async, throwRuntimeError); + auto status = fut.wait_for(1s); // shouldn't run for more than 1 second + if(status != std::future_status::ready) + { + std::cerr << "Test took too long. Possible infinite loop.\n"; + std::exit(EXIT_FAILURE); + } + std::exit(EXIT_SUCCESS); + }, + ::testing::ExitedWithCode(EXIT_SUCCESS), ".*"); } From 3ea8fc4aa1dd7e5625b499c0d2164a65b883deae Mon Sep 17 00:00:00 2001 From: alvintps Date: Sun, 18 Jan 2026 14:30:37 +0800 Subject: [PATCH 4/6] Added tsan_suppressions file to suppress TSAN warnings from zeromq --- .github/workflows/cmake_ubuntu_sanitizers.yml | 2 +- tests/tsan_suppressions.txt | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 tests/tsan_suppressions.txt diff --git a/.github/workflows/cmake_ubuntu_sanitizers.yml b/.github/workflows/cmake_ubuntu_sanitizers.yml index 337bc79bb..78bbb62d4 100644 --- a/.github/workflows/cmake_ubuntu_sanitizers.yml +++ b/.github/workflows/cmake_ubuntu_sanitizers.yml @@ -63,7 +63,7 @@ jobs: GTEST_COLOR: "On" ASAN_OPTIONS: "color=always" UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always" - TSAN_OPTIONS: "color=always" + TSAN_OPTIONS: "suppressions=${GITHUB_WORKSPACE}/tests/tsan_suppressions.txt:color=always" # There is a known issue with TSAN on recent kernel versions. Without the vm.mmap_rnd_bits=28 # workaround all binaries with TSan enabled crash with "FATAL: ThreadSanitizer: unexpected memory mapping" run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure diff --git a/tests/tsan_suppressions.txt b/tests/tsan_suppressions.txt new file mode 100644 index 000000000..e045f985d --- /dev/null +++ b/tests/tsan_suppressions.txt @@ -0,0 +1,4 @@ +# ThreadSanitizer suppressions file for behaviortree_cpp_test + +# ZeroMQ false positives +race:zmq::epoll_t::add_fd From 798fa2bd69adb7d86b80e4d8de4fe962881f79bd Mon Sep 17 00:00:00 2001 From: alvintps Date: Sun, 18 Jan 2026 15:02:32 +0800 Subject: [PATCH 5/6] Replaced atomics with mutex for type safety with chrono --- src/loggers/groot2_publisher.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/loggers/groot2_publisher.cpp b/src/loggers/groot2_publisher.cpp index 1880818c0..22219b732 100644 --- a/src/loggers/groot2_publisher.cpp +++ b/src/loggers/groot2_publisher.cpp @@ -98,7 +98,8 @@ struct Groot2Publisher::PImpl std::unordered_map pre_hooks; std::unordered_map post_hooks; - std::atomic last_heartbeat; + std::mutex last_heartbeat_mutex; + std::chrono::steady_clock::time_point last_heartbeat; std::chrono::milliseconds max_heartbeat_delay = std::chrono::milliseconds(5000); std::atomic_bool recording = false; @@ -251,7 +252,10 @@ void Groot2Publisher::serverLoop() }; // initialize _p->last_heartbeat - _p->last_heartbeat = std::chrono::steady_clock::now().time_since_epoch().count(); + { + const std::unique_lock lk(_p->last_heartbeat_mutex); + _p->last_heartbeat = std::chrono::steady_clock::now(); + } while(_p->active_server) { @@ -260,8 +264,12 @@ void Groot2Publisher::serverLoop() { continue; } + // this heartbeat will help establishing if Groot is connected or not - _p->last_heartbeat = std::chrono::steady_clock::now().time_since_epoch().count(); + { + const std::unique_lock lk(_p->last_heartbeat_mutex); + _p->last_heartbeat = std::chrono::steady_clock::now(); + } std::string const request_str = requestMsg[0].to_string(); if(request_str.size() != Monitor::RequestHeader::size()) @@ -514,8 +522,10 @@ void Groot2Publisher::heartbeatLoop() auto now = std::chrono::steady_clock::now(); const bool prev_heartbeat = has_heartbeat; - has_heartbeat = ((now.time_since_epoch().count() - _p->last_heartbeat) < - _p->max_heartbeat_delay.count()); + { + const std::unique_lock lk(_p->last_heartbeat_mutex); + has_heartbeat = (now - _p->last_heartbeat < _p->max_heartbeat_delay); + } // if we loose or gain heartbeat, disable/enable all breakpoints if(has_heartbeat != prev_heartbeat) From de7cd0ce2b5c442b13ffd260bcbdbbafe66e7a98 Mon Sep 17 00:00:00 2001 From: alvintps Date: Sun, 18 Jan 2026 15:29:37 +0800 Subject: [PATCH 6/6] Fixed suppressions path --- .github/workflows/cmake_ubuntu_sanitizers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cmake_ubuntu_sanitizers.yml b/.github/workflows/cmake_ubuntu_sanitizers.yml index 78bbb62d4..15f931166 100644 --- a/.github/workflows/cmake_ubuntu_sanitizers.yml +++ b/.github/workflows/cmake_ubuntu_sanitizers.yml @@ -63,7 +63,7 @@ jobs: GTEST_COLOR: "On" ASAN_OPTIONS: "color=always" UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always" - TSAN_OPTIONS: "suppressions=${GITHUB_WORKSPACE}/tests/tsan_suppressions.txt:color=always" + TSAN_OPTIONS: "suppressions=../../../tests/tsan_suppressions.txt:color=always" # There is a known issue with TSAN on recent kernel versions. Without the vm.mmap_rnd_bits=28 # workaround all binaries with TSan enabled crash with "FATAL: ThreadSanitizer: unexpected memory mapping" run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure