From 1ed4f047946a91293dcec4a47f3df90df0669c64 Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Mon, 6 Oct 2025 16:54:36 +0000 Subject: [PATCH 01/10] Add support for sanitizers including some GHAs --- .github/workflows/cmake_ubuntu_aubsan.yml | 57 +++++++++++++++++++++++ .github/workflows/cmake_ubuntu_tsan.yml | 56 ++++++++++++++++++++++ CMakeLists.txt | 5 ++ cmake/sanitizers.cmake | 30 ++++++++++++ 4 files changed, 148 insertions(+) create mode 100644 .github/workflows/cmake_ubuntu_aubsan.yml create mode 100644 .github/workflows/cmake_ubuntu_tsan.yml create mode 100644 cmake/sanitizers.cmake diff --git a/.github/workflows/cmake_ubuntu_aubsan.yml b/.github/workflows/cmake_ubuntu_aubsan.yml new file mode 100644 index 000000000..13db9ff15 --- /dev/null +++ b/.github/workflows/cmake_ubuntu_aubsan.yml @@ -0,0 +1,57 @@ +name: cmake Ubuntu with Address and Undefined Behavior Sanitizers + +on: + push: + branches: + - master + pull_request: + types: [opened, synchronize, reopened] + +env: + # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.) + BUILD_TYPE: Debug + +jobs: + build: + # The CMake configure and build commands are platform agnostic and should work equally + # well on Windows or Mac. You can convert this to a matrix build if you need + # cross-platform coverage. + # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix + runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-22.04] + + steps: + - uses: actions/checkout@v2 + + - name: Install Conan + id: conan + uses: turtlebrowser/get-conan@main + + - name: Create default profile + run: conan profile detect + + - name: Install conan dependencies + run: conan install conanfile.py -s build_type=${{env.BUILD_TYPE}} --build=missing + + - name: Normalize build type + shell: bash + # The build type is Capitalized, e.g. Release, but the preset is all lowercase, e.g. release. + # There is no built in way to do string manipulations on GHA as far as I know.` + run: echo "BUILD_TYPE_LOWERCASE=$(echo "${BUILD_TYPE}" | tr '[:upper:]' '[:lower:]')" >> $GITHUB_ENV + + - name: Configure CMake + shell: bash + run: cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} -DBTCPP_ENABLE_ASAN:BOOL=ON -DBTCPP_ENABLE_UBSAN:BOOL=ON + + - name: Build + shell: bash + run: cmake --build --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} + + - name: run test (Linux + Address and Undefined Behavior Sanitizers) + env: + GTEST_COLOR: "On" + ASAN_OPTIONS: "color=always" + UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always" + run: ctest --test-dir build/${{env.BUILD_TYPE}} diff --git a/.github/workflows/cmake_ubuntu_tsan.yml b/.github/workflows/cmake_ubuntu_tsan.yml new file mode 100644 index 000000000..197f18ebe --- /dev/null +++ b/.github/workflows/cmake_ubuntu_tsan.yml @@ -0,0 +1,56 @@ +name: cmake Ubuntu with Thread Sanitizer + +on: + push: + branches: + - master + pull_request: + types: [opened, synchronize, reopened] + +env: + # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.) + BUILD_TYPE: Debug + +jobs: + build: + # The CMake configure and build commands are platform agnostic and should work equally + # well on Windows or Mac. You can convert this to a matrix build if you need + # cross-platform coverage. + # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix + runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-22.04] + + steps: + - uses: actions/checkout@v2 + + - name: Install Conan + id: conan + uses: turtlebrowser/get-conan@main + + - name: Create default profile + run: conan profile detect + + - name: Install conan dependencies + run: conan install conanfile.py -s build_type=${{env.BUILD_TYPE}} --build=missing + + - name: Normalize build type + shell: bash + # The build type is Capitalized, e.g. Release, but the preset is all lowercase, e.g. release. + # There is no built in way to do string manipulations on GHA as far as I know.` + run: echo "BUILD_TYPE_LOWERCASE=$(echo "${BUILD_TYPE}" | tr '[:upper:]' '[:lower:]')" >> $GITHUB_ENV + + - name: Configure CMake + shell: bash + run: cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} -DBTCPP_ENABLE_TSAN:BOOL=ON + + - name: Build + shell: bash + run: cmake --build --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} + + - name: run test (Linux + Thread Sanitizer) + env: + GTEST_COLOR: "On" + TSAN_OPTIONS: "color=always" + run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} diff --git a/CMakeLists.txt b/CMakeLists.txt index a0cb7b202..320877a91 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,6 +13,9 @@ option(BTCPP_EXAMPLES "Build tutorials and examples" ON) option(BUILD_TESTING "Build the unit tests" ON) option(BTCPP_GROOT_INTERFACE "Add Groot2 connection. Requires ZeroMQ" ON) option(BTCPP_SQLITE_LOGGING "Add SQLite logging." ON) +option(BTCPP_ENABLE_ASAN "Enable Address Sanitizer" OFF) +option(BTCPP_ENABLE_UBSAN "Enable Undefined Behavior Sanitizer" OFF) +option(BTCPP_ENABLE_TSAN "Enable Thread Sanitizer" OFF) option(USE_V3_COMPATIBLE_NAMES "Use some alias to compile more easily old 3.x code" OFF) option(ENABLE_FUZZING "Enable fuzzing builds" OFF) @@ -54,6 +57,8 @@ endif() set(CMAKE_CONFIG_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_LIST_DIR}/cmake") list(APPEND CMAKE_MODULE_PATH "${CMAKE_CONFIG_PATH}") +include(sanitizers) + set(BTCPP_LIBRARY ${PROJECT_NAME}) if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES) diff --git a/cmake/sanitizers.cmake b/cmake/sanitizers.cmake new file mode 100644 index 000000000..fdb542e68 --- /dev/null +++ b/cmake/sanitizers.cmake @@ -0,0 +1,30 @@ +if(BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN OR BTCPP_ENABLE_TSAN) + if(NOT CMAKE_BUILD_TYPE MATCHES "Debug|RelWithDebInfo") + message(FATAL_ERROR "Sanitizers require debug symbols. Please set CMAKE_BUILD_TYPE to Debug or RelWithDebInfo.") + endif() + add_compile_options(-fno-omit-frame-pointer) +endif() + +# Address Sanitizer and Undefined Behavior Sanitizer can be run at the same time. +# Thread Sanitizer requires its own build. +if(BTCPP_ENABLE_TSAN AND (BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN)) + message(FATAL_ERROR "TSAN is not compatible with ASAN or UBSAN. Please enable only one of them.") +endif() + +if(BTCPP_ENABLE_ASAN) + message(STATUS "Address Sanitizer enabled") + add_compile_options(-fsanitize=address) + add_link_options(-fsanitize=address) +endif() + +if(BTCPP_ENABLE_UBSAN) + message(STATUS "Undefined Behavior Sanitizer enabled") + add_compile_options(-fsanitize=undefined) + add_link_options(-fsanitize=undefined) +endif() + +if(BTCPP_ENABLE_TSAN) + message(STATUS "Thread Sanitizer enabled") + add_compile_options(-fsanitize=thread) + add_link_options(-fsanitize=thread) +endif() From 92feada203d31f94bb1c85973811d125149a4500 Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Tue, 7 Oct 2025 18:24:50 +0000 Subject: [PATCH 02/10] use gtest_discover_tests to regiester the unit tests this modern approach registers many individual tests instead of a single monolitic test so if one fails the rest continue running which allows the developer to flag multiple failing tests on a single run It also speeds up testing since tests run in parallel --- tests/CMakeLists.txt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9c62b335e..56a03e501 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -43,16 +43,19 @@ if(ament_cmake_FOUND) else() + enable_testing() + find_package(GTest REQUIRED) + include(GoogleTest) - enable_testing() add_executable(behaviortree_cpp_test ${BT_TESTS}) - add_test(NAME btcpp_test COMMAND behaviortree_cpp_test) target_link_libraries(behaviortree_cpp_test GTest::gtest GTest::gtest_main) + gtest_discover_tests(behaviortree_cpp_test) + endif() target_include_directories(behaviortree_cpp_test PRIVATE include) From 2523528fcd68b197f2604fda5266d3db6a478eeb Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Tue, 7 Oct 2025 18:55:31 +0000 Subject: [PATCH 03/10] Combine sanitizer actions into a single file --- ...aubsan.yml => cmake_ubuntu_sanitizers.yml} | 15 ++++- .github/workflows/cmake_ubuntu_tsan.yml | 56 ------------------- 2 files changed, 12 insertions(+), 59 deletions(-) rename .github/workflows/{cmake_ubuntu_aubsan.yml => cmake_ubuntu_sanitizers.yml} (75%) delete mode 100644 .github/workflows/cmake_ubuntu_tsan.yml diff --git a/.github/workflows/cmake_ubuntu_aubsan.yml b/.github/workflows/cmake_ubuntu_sanitizers.yml similarity index 75% rename from .github/workflows/cmake_ubuntu_aubsan.yml rename to .github/workflows/cmake_ubuntu_sanitizers.yml index 13db9ff15..4c0b3bd17 100644 --- a/.github/workflows/cmake_ubuntu_aubsan.yml +++ b/.github/workflows/cmake_ubuntu_sanitizers.yml @@ -1,4 +1,4 @@ -name: cmake Ubuntu with Address and Undefined Behavior Sanitizers +name: cmake Ubuntu Sanitizers on: push: @@ -21,6 +21,7 @@ jobs: strategy: matrix: os: [ubuntu-22.04] + sanitizer: [asan_ubsan, tsan] steps: - uses: actions/checkout@v2 @@ -43,7 +44,14 @@ jobs: - name: Configure CMake shell: bash - run: cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} -DBTCPP_ENABLE_ASAN:BOOL=ON -DBTCPP_ENABLE_UBSAN:BOOL=ON + run: | + if [[ "${{ matrix.sanitizer }}" == "asan_ubsan" ]]; then + cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} \ + -DBTCPP_ENABLE_ASAN:BOOL=ON -DBTCPP_ENABLE_UBSAN:BOOL=ON + else + cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} \ + -DBTCPP_ENABLE_TSAN:BOOL=ON + fi - name: Build shell: bash @@ -54,4 +62,5 @@ jobs: GTEST_COLOR: "On" ASAN_OPTIONS: "color=always" UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always" - run: ctest --test-dir build/${{env.BUILD_TYPE}} + TSAN_OPTIONS: "color=always" + run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure diff --git a/.github/workflows/cmake_ubuntu_tsan.yml b/.github/workflows/cmake_ubuntu_tsan.yml deleted file mode 100644 index 197f18ebe..000000000 --- a/.github/workflows/cmake_ubuntu_tsan.yml +++ /dev/null @@ -1,56 +0,0 @@ -name: cmake Ubuntu with Thread Sanitizer - -on: - push: - branches: - - master - pull_request: - types: [opened, synchronize, reopened] - -env: - # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.) - BUILD_TYPE: Debug - -jobs: - build: - # The CMake configure and build commands are platform agnostic and should work equally - # well on Windows or Mac. You can convert this to a matrix build if you need - # cross-platform coverage. - # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix - runs-on: ${{ matrix.os }} - strategy: - matrix: - os: [ubuntu-22.04] - - steps: - - uses: actions/checkout@v2 - - - name: Install Conan - id: conan - uses: turtlebrowser/get-conan@main - - - name: Create default profile - run: conan profile detect - - - name: Install conan dependencies - run: conan install conanfile.py -s build_type=${{env.BUILD_TYPE}} --build=missing - - - name: Normalize build type - shell: bash - # The build type is Capitalized, e.g. Release, but the preset is all lowercase, e.g. release. - # There is no built in way to do string manipulations on GHA as far as I know.` - run: echo "BUILD_TYPE_LOWERCASE=$(echo "${BUILD_TYPE}" | tr '[:upper:]' '[:lower:]')" >> $GITHUB_ENV - - - name: Configure CMake - shell: bash - run: cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} -DBTCPP_ENABLE_TSAN:BOOL=ON - - - name: Build - shell: bash - run: cmake --build --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} - - - name: run test (Linux + Thread Sanitizer) - env: - GTEST_COLOR: "On" - TSAN_OPTIONS: "color=always" - run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} From 1b989f35fbf711a3ec65534a408b8b731db541b1 Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Tue, 7 Oct 2025 19:22:29 +0000 Subject: [PATCH 04/10] Do not fail fast. We want results of both sanitizer runs --- .github/workflows/cmake_ubuntu_sanitizers.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/cmake_ubuntu_sanitizers.yml b/.github/workflows/cmake_ubuntu_sanitizers.yml index 4c0b3bd17..e14a34e47 100644 --- a/.github/workflows/cmake_ubuntu_sanitizers.yml +++ b/.github/workflows/cmake_ubuntu_sanitizers.yml @@ -19,6 +19,7 @@ jobs: # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: os: [ubuntu-22.04] sanitizer: [asan_ubsan, tsan] From 390b99bb79ee637a9bbf52eaf7182b77aa055c41 Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Tue, 7 Oct 2025 19:03:47 +0000 Subject: [PATCH 05/10] Fix windows builds --- tests/CMakeLists.txt | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 56a03e501..b268235df 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -54,7 +54,16 @@ else() GTest::gtest GTest::gtest_main) - gtest_discover_tests(behaviortree_cpp_test) + # gtest_discover_tests queries the test executable for available tests and registers them on ctest individually + # On Windows it needs a little help to find the shared libraries + if(WIN32) + gtest_discover_tests(behaviortree_cpp_test + DISCOVERY_MODE PRE_TEST + DISCOVERY_ENVIRONMENT "PATH=$;$ENV{PATH}" + ) + else() + gtest_discover_tests(behaviortree_cpp_test) + endif() endif() From d6221b0435fe6444257ab9bbe52bf5942613afa8 Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Tue, 7 Oct 2025 19:40:25 +0000 Subject: [PATCH 06/10] Leave a note for posterity --- .github/workflows/cmake_ubuntu_sanitizers.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/cmake_ubuntu_sanitizers.yml b/.github/workflows/cmake_ubuntu_sanitizers.yml index e14a34e47..c379171a1 100644 --- a/.github/workflows/cmake_ubuntu_sanitizers.yml +++ b/.github/workflows/cmake_ubuntu_sanitizers.yml @@ -64,4 +64,6 @@ jobs: ASAN_OPTIONS: "color=always" UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always" TSAN_OPTIONS: "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 From 24f3d93ceb8fa1d71a312c1910a21fe597bed316 Mon Sep 17 00:00:00 2001 From: Eric Riff Date: Tue, 7 Oct 2025 19:46:22 +0000 Subject: [PATCH 07/10] Improve error message --- cmake/sanitizers.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmake/sanitizers.cmake b/cmake/sanitizers.cmake index fdb542e68..347c2aa0d 100644 --- a/cmake/sanitizers.cmake +++ b/cmake/sanitizers.cmake @@ -8,7 +8,7 @@ endif() # Address Sanitizer and Undefined Behavior Sanitizer can be run at the same time. # Thread Sanitizer requires its own build. if(BTCPP_ENABLE_TSAN AND (BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN)) - message(FATAL_ERROR "TSAN is not compatible with ASAN or UBSAN. Please enable only one of them.") + message(FATAL_ERROR "TSan is not compatible with ASan or UBSan. ASan and UBSan can run together, but TSan requires its own separate build.") endif() if(BTCPP_ENABLE_ASAN) From 3d7e1e1fe2106b3adb10a6b28b779023c71f5211 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Tue, 14 Oct 2025 20:03:39 +0200 Subject: [PATCH 08/10] fix thread safety issues --- cmake/sanitizers.cmake | 9 +++--- include/behaviortree_cpp/blackboard.h | 12 ++++--- include/behaviortree_cpp/utils/timer_queue.h | 34 ++++++++++++-------- src/blackboard.cpp | 20 +++++++----- tests/gtest_blackboard.cpp | 3 ++ 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/cmake/sanitizers.cmake b/cmake/sanitizers.cmake index 347c2aa0d..fa5dccc84 100644 --- a/cmake/sanitizers.cmake +++ b/cmake/sanitizers.cmake @@ -15,16 +15,17 @@ if(BTCPP_ENABLE_ASAN) message(STATUS "Address Sanitizer enabled") add_compile_options(-fsanitize=address) add_link_options(-fsanitize=address) -endif() + endif() -if(BTCPP_ENABLE_UBSAN) + if(BTCPP_ENABLE_UBSAN) message(STATUS "Undefined Behavior Sanitizer enabled") add_compile_options(-fsanitize=undefined) add_link_options(-fsanitize=undefined) -endif() + endif() -if(BTCPP_ENABLE_TSAN) + if(BTCPP_ENABLE_TSAN) message(STATUS "Thread Sanitizer enabled") add_compile_options(-fsanitize=thread) add_link_options(-fsanitize=thread) + add_compile_definitions(USE_SANITIZE_THREAD) endif() diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index 1c3aa96c6..412360bf8 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -141,7 +141,7 @@ class Blackboard const Blackboard* rootBlackboard() const; private: - mutable std::mutex mutex_; + mutable std::mutex storage_mutex_; mutable std::recursive_mutex entry_mutex_; std::unordered_map> storage_; std::weak_ptr parent_bb_; @@ -186,7 +186,7 @@ inline T Blackboard::get(const std::string& key) const inline void Blackboard::unset(const std::string& key) { - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); // check local storage auto it = storage_.find(key); @@ -207,7 +207,7 @@ inline void Blackboard::set(const std::string& key, const T& value) rootBlackboard()->set(key.substr(1, key.size() - 1), value); return; } - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); // check local storage auto it = storage_.find(key); @@ -215,7 +215,7 @@ inline void Blackboard::set(const std::string& key, const T& value) { // create a new entry Any new_value(value); - lock.unlock(); + storage_lock.unlock(); std::shared_ptr entry; // if a new generic port is created with a string, it's type should be AnyTypeAllowed if constexpr(std::is_same_v) @@ -228,7 +228,7 @@ inline void Blackboard::set(const std::string& key, const T& value) GetAnyFromStringFunctor()); entry = createEntryImpl(key, new_port); } - lock.lock(); + storage_lock.lock(); entry->value = new_value; entry->sequence_id++; @@ -239,6 +239,8 @@ inline void Blackboard::set(const std::string& key, const T& value) // this is not the first time we set this entry, we need to check // if the type is the same or not. Entry& entry = *it->second; + storage_lock.unlock(); + std::scoped_lock scoped_lock(entry.entry_mutex); Any& previous_any = entry.value; diff --git a/include/behaviortree_cpp/utils/timer_queue.h b/include/behaviortree_cpp/utils/timer_queue.h index 9132277ba..56738ff56 100644 --- a/include/behaviortree_cpp/utils/timer_queue.h +++ b/include/behaviortree_cpp/utils/timer_queue.h @@ -23,10 +23,7 @@ class Semaphore void notify() { - { - std::lock_guard lock(m_mtx); - m_count++; - } + m_count.fetch_add(1); m_cv.notify_one(); } @@ -38,8 +35,15 @@ class Semaphore { return false; } - m_count--; + // Only decrement if there is a real count. If we woke because of manualUnlock, + // m_count may be zero and we must not decrement it. + if(m_count > 0) + { + m_count.fetch_sub(1); + } + // Clear the manual unlock flag m_unlock = false; + return true; } @@ -52,7 +56,7 @@ class Semaphore private: std::mutex m_mtx; std::condition_variable m_cv; - unsigned m_count = 0; + std::atomic_uint m_count = 0; std::atomic_bool m_unlock = false; }; } // namespace details @@ -74,15 +78,19 @@ class TimerQueue public: TimerQueue() { - m_th = std::thread([this] { run(); }); + m_finish.store(false); + m_thread = std::thread([this]() { run(); }); } ~TimerQueue() { - m_finish = true; + m_finish.store(true); cancelAll(); - m_checkWork.manualUnlock(); - m_th.join(); + + if(m_thread.joinable()) + { + m_thread.join(); + } } //! Adds a new timer @@ -174,7 +182,7 @@ class TimerQueue void run() { - while(!m_finish) + while(!m_finish.load()) { auto end = calcWaitTime(); if(end.first) @@ -239,8 +247,8 @@ class TimerQueue } details::Semaphore m_checkWork; - std::thread m_th; - bool m_finish = false; + std::thread m_thread; + std::atomic_bool m_finish = false; uint64_t m_idcounter = 0; struct WorkItem diff --git a/src/blackboard.cpp b/src/blackboard.cpp index 3b1ba9844..462e7b41e 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -52,11 +52,13 @@ Blackboard::getEntry(const std::string& key) const return rootBlackboard()->getEntry(key.substr(1, key.size() - 1)); } - std::unique_lock lock(mutex_); - auto it = storage_.find(key); - if(it != storage_.end()) { - return it->second; + std::unique_lock storage_lock(storage_mutex_); + auto it = storage_.find(key); + if(it != storage_.end()) + { + return it->second; + } } // not found. Try autoremapping if(auto parent = parent_bb_.lock()) @@ -130,7 +132,7 @@ std::vector Blackboard::getKeys() const void Blackboard::clear() { - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); storage_.clear(); } @@ -157,8 +159,10 @@ void Blackboard::createEntry(const std::string& key, const TypeInfo& info) void Blackboard::cloneInto(Blackboard& dst) const { - std::unique_lock lk1(mutex_); - std::unique_lock lk2(dst.mutex_); + // Lock both mutexes without risking lock-order inversion. + std::unique_lock lk1(storage_mutex_, std::defer_lock); + std::unique_lock lk2(dst.storage_mutex_, std::defer_lock); + std::lock(lk1, lk2); // keys that are not updated must be removed. std::unordered_set keys_to_remove; @@ -212,7 +216,7 @@ Blackboard::Ptr Blackboard::parent() std::shared_ptr Blackboard::createEntryImpl(const std::string& key, const TypeInfo& info) { - std::unique_lock lock(mutex_); + std::unique_lock storage_lock(storage_mutex_); // This function might be called recursively, when we do remapping, because we move // to the top scope to find already existing entries diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index 35be37dab..75c3db6d1 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -294,6 +294,8 @@ TEST(BlackboardTest, CheckTypeSafety) ASSERT_TRUE(is); } +#ifndef USE_SANITIZE_THREAD + TEST(BlackboardTest, AnyPtrLocked) { auto blackboard = Blackboard::create(); @@ -346,6 +348,7 @@ TEST(BlackboardTest, AnyPtrLocked) ASSERT_NE(cycles, value); } } +#endif TEST(BlackboardTest, SetStringView) { From 06c856af3d91bef52e1c96a4fc7d48f29211912d Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Tue, 14 Oct 2025 20:19:16 +0200 Subject: [PATCH 09/10] fix memory leak --- src/tree_node.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/tree_node.cpp b/src/tree_node.cpp index 0d4dd66bd..946492318 100644 --- a/src/tree_node.cpp +++ b/src/tree_node.cpp @@ -106,19 +106,13 @@ NodeStatus TreeNode::executeTick() if(!substituted) { using namespace std::chrono; - - auto t1 = steady_clock::now(); - // trick to prevent the compile from reordering the order of execution. See #861 - // This makes sure that the code is executed at the end of this scope - std::shared_ptr execute_later(nullptr, [&](...) { - auto t2 = steady_clock::now(); - if(monitor_tick) - { - monitor_tick(*this, new_status, duration_cast(t2 - t1)); - } - }); - + const auto t1 = steady_clock::now(); new_status = tick(); + const auto t2 = steady_clock::now(); + if(monitor_tick) + { + monitor_tick(*this, new_status, duration_cast(t2 - t1)); + } } } From 831cadd0972e8ca66692ee7c053b7ede585fe831 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Tue, 14 Oct 2025 20:38:28 +0200 Subject: [PATCH 10/10] fix issue in destruction order --- include/behaviortree_cpp/utils/timer_queue.h | 1 - tests/gtest_decorator.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/include/behaviortree_cpp/utils/timer_queue.h b/include/behaviortree_cpp/utils/timer_queue.h index 56738ff56..2a68ba872 100644 --- a/include/behaviortree_cpp/utils/timer_queue.h +++ b/include/behaviortree_cpp/utils/timer_queue.h @@ -86,7 +86,6 @@ class TimerQueue { m_finish.store(true); cancelAll(); - if(m_thread.joinable()) { m_thread.join(); diff --git a/tests/gtest_decorator.cpp b/tests/gtest_decorator.cpp index 32abcff10..fde33ea9b 100644 --- a/tests/gtest_decorator.cpp +++ b/tests/gtest_decorator.cpp @@ -68,8 +68,8 @@ struct RetryTest : testing::Test struct TimeoutAndRetry : testing::Test { - BT::TimeoutNode timeout_root; BT::RetryNode retry; + BT::TimeoutNode timeout_root; BT::SyncActionTest action; TimeoutAndRetry() : timeout_root("deadline", 9), retry("retry", 1000), action("action")