Fixed possible infinite loop in Groot2Publisher when destructor is ca… #1058
Conversation
📝 WalkthroughWalkthroughGroot2Publisher now defaults its server-active flag to true and switches heartbeat timing to a mutex-protected Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ServerLoop
participant HeartbeatLoop
participant SharedHeartbeat as "Shared last_heartbeat\n(mutex + steady_clock)"
participant Publisher
Note over Publisher: Publisher._p->active_server defaults to true
Client->>ServerLoop: request/operation
ServerLoop->>SharedHeartbeat: lock & read last_heartbeat
SharedHeartbeat-->>ServerLoop: last_heartbeat (unlock)
ServerLoop->>Publisher: make decision based on heartbeat
HeartbeatLoop->>SharedHeartbeat: lock, set last_heartbeat = steady_clock::now()
SharedHeartbeat-->>HeartbeatLoop: unlock
HeartbeatLoop->>Publisher: emit heartbeat / health update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alvintps
force-pushed
the
master
branch
2 times, most recently
from
0c1629a to
0456e24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/gtest_groot2_publisher.cpp`:
- Around line 34-39: The test EnsureNoInfiniteLoopOnThrow currently launches
throwRuntimeError with std::async which can block in the std::future destructor;
replace this with a subprocess-based approach: execute throwRuntimeError in a
child process (e.g., fork/exec or spawning a helper binary) from the test, have
the parent wait with a timeout, on timeout kill the child and fail the test, and
on child exit check its exit status to assert behavior. Locate the test by name
EnsureNoInfiniteLoopOnThrow and the target function throwRuntimeError and
implement the fork/waitpid + alarm/timeout (or spawn helper process) pattern so
the test runner cannot be blocked by a hanging async task.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/loggers/groot2_publisher.cpptests/gtest_groot2_publisher.cpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: C++ code must follow Google C++ style with 90-character line limit and 2-space indent
All C++ code must be processed by clang-format before submission
Run clang-tidy-21 checks on C++ code before submitting PRs
Files:
tests/gtest_groot2_publisher.cppsrc/loggers/groot2_publisher.cpp
tests/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must use Google Test framework and be located in the tests/ directory
Files:
tests/gtest_groot2_publisher.cpp
tests/gtest_*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
All test files must be runnable via CTest and should follow the naming convention of gtest_*.cpp for categorized test files
Files:
tests/gtest_groot2_publisher.cpp
src/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
All behavior tree nodes must inherit from TreeNode base class defined in include/behaviortree_cpp/tree_node.h
Files:
src/loggers/groot2_publisher.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-09T08:28:16.736Z
Learnt from: CR
Repo: BehaviorTree/BehaviorTree.CPP PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T08:28:16.736Z
Learning: Applies to tests/gtest_*.cpp : All test files must be runnable via CTest and should follow the naming convention of gtest_*.cpp for categorized test files
Applied to files:
tests/gtest_groot2_publisher.cpp
📚 Learning: 2026-01-09T08:28:16.736Z
Learnt from: CR
Repo: BehaviorTree/BehaviorTree.CPP PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T08:28:16.736Z
Learning: Applies to tests/**/*.{cpp,h,hpp} : Test files must use Google Test framework and be located in the tests/ directory
Applied to files:
tests/gtest_groot2_publisher.cpp
🧬 Code graph analysis (1)
tests/gtest_groot2_publisher.cpp (1)
include/behaviortree_cpp/exceptions.h (1)
RuntimeError(59-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-22.04, tsan)
- GitHub Check: industrial_ci (humble, main)
- GitHub Check: build (ubuntu-22.04, asan_ubsan)
- GitHub Check: industrial_ci (rolling, main)
- GitHub Check: build (windows-latest)
- GitHub Check: industrial_ci (jazzy, main)
- GitHub Check: pixi_conda_build (windows-latest)
- GitHub Check: clang-tidy
- GitHub Check: pixi_conda_build (ubuntu-latest)
🔇 Additional comments (2)
src/loggers/groot2_publisher.cpp (1)
84-84: Active server init aligns with shutdown flow.Initializing
BT::Groot2Publisher::PImpl::active_servertotrueand only clearing it in the destructor avoids the loop reactivating itself.tests/gtest_groot2_publisher.cpp (1)
20-31: Helper covers the exception path well.Nice, focused regression setup for the runtime-error tick and publisher teardown.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/gtest_groot2_publisher.cpp`:
- Around line 1-7: Add the missing header declaration for std::exit by including
<cstdlib> in the test translation unit; update the top of
tests/gtest_groot2_publisher.cpp so that std::exit is declared (used where
std::exit is called around the test teardown), ensuring compilation doesn't rely
on transitive includes.
🧹 Nitpick comments (1)
tests/gtest_groot2_publisher.cpp (1)
36-43: Consider a more generous timeout to reduce CI flakiness.
wait_for(1s)may be tight on busy/slow runners. A longer or configurable timeout would make the test more robust.🔧 Suggested change
- auto status = fut.wait_for(1s); + auto status = fut.wait_for(5s);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/loggers/groot2_publisher.cpptests/CMakeLists.txttests/gtest_groot2_publisher.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/loggers/groot2_publisher.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: C++ code must follow Google C++ style with 90-character line limit and 2-space indent
All C++ code must be processed by clang-format before submission
Run clang-tidy-21 checks on C++ code before submitting PRs
Files:
tests/gtest_groot2_publisher.cpp
tests/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must use Google Test framework and be located in the tests/ directory
Files:
tests/gtest_groot2_publisher.cpp
tests/gtest_*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
All test files must be runnable via CTest and should follow the naming convention of gtest_*.cpp for categorized test files
Files:
tests/gtest_groot2_publisher.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-09T08:28:16.736Z
Learnt from: CR
Repo: BehaviorTree/BehaviorTree.CPP PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T08:28:16.736Z
Learning: Applies to tests/gtest_*.cpp : All test files must be runnable via CTest and should follow the naming convention of gtest_*.cpp for categorized test files
Applied to files:
tests/gtest_groot2_publisher.cpptests/CMakeLists.txt
📚 Learning: 2026-01-09T08:28:16.736Z
Learnt from: CR
Repo: BehaviorTree/BehaviorTree.CPP PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T08:28:16.736Z
Learning: Applies to tests/**/*.{cpp,h,hpp} : Test files must use Google Test framework and be located in the tests/ directory
Applied to files:
tests/gtest_groot2_publisher.cpptests/CMakeLists.txt
🧬 Code graph analysis (1)
tests/gtest_groot2_publisher.cpp (1)
include/behaviortree_cpp/exceptions.h (1)
RuntimeError(59-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pixi_conda_build (ubuntu-latest)
- GitHub Check: pixi_conda_build (windows-latest)
- GitHub Check: clang-tidy
- GitHub Check: industrial_ci (rolling, main)
- GitHub Check: industrial_ci (humble, main)
- GitHub Check: industrial_ci (jazzy, main)
- GitHub Check: build (ubuntu-22.04, asan_ubsan)
- GitHub Check: build (ubuntu-22.04, tsan)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (1)
tests/CMakeLists.txt (1)
14-16: LGTM — test is properly registered.Adding the new test file to
BT_TESTSis correct for discovery and build.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
thread sanitizer looks unhappy 😃 https://github.com/BehaviorTree/BehaviorTree.CPP/actions/runs/21073716797/job/60609993659?pr=1058 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/cmake_ubuntu_sanitizers.yml:
- Line 66: The TSAN suppressions path in the TSAN_OPTIONS env var is wrong;
update the value referenced by TSAN_OPTIONS (the line defining TSAN_OPTIONS) to
point to ${GITHUB_WORKSPACE}/tests/tsan_suppressions.txt instead of
${GITHUB_WORKSPACE}/BehaviorTree.CPP/tests/tsan_suppressions.txt so the workflow
uses the actual suppressions file location after actions/checkout@v6.
In `@src/loggers/groot2_publisher.cpp`:
- Around line 514-518: The heartbeat comparison mixes raw clock ticks with a
millisecond duration; fix it by reconstructing a steady_clock time_point or
duration from _p->last_heartbeat and then use chrono's type-safe
subtraction/comparison. For example, convert _p->last_heartbeat into a
std::chrono::steady_clock::time_point (or wrap the raw difference into a
std::chrono::steady_clock::duration) and then replace the current expression in
groot2_publisher.cpp (the has_heartbeat calculation using now,
_p->last_heartbeat and _p->max_heartbeat_delay) with a comparison like (now -
last_time_point) < _p->max_heartbeat_delay so units are handled correctly;
update uses of has_heartbeat, _p->last_heartbeat, and _p->max_heartbeat_delay
accordingly.
|
It's happy now 😏 |
2 participants
This PR addresses Issue #1057.
active_serveris no longer set totruein the server loop. Instead it istrueon construction, and set tofalseon destruction.Summary by CodeRabbit
Bug Fixes
Tests
Chores / CI
✏️ Tip: You can customize this high-level summary in your review settings.