Fixed possible infinite loop in Groot2Publisher when destructor is ca… #1058
Conversation
📝 WalkthroughWalkthroughGroot2Publisher now default-initializes its internal Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
| #include <chrono> | ||
| #include <future> | ||
|
|
||
| #include <behaviortree_cpp/loggers/groot2_protocol.h> | ||
| #include <behaviortree_cpp/loggers/groot2_publisher.h> | ||
| #include <gtest/gtest.h> | ||
|
|
There was a problem hiding this comment.
Add the missing <cstdlib> include for std::exit.
Line 42 uses std::exit, which should be declared in <cstdlib> rather than relying on transitive includes.
🔧 Suggested change
`#include` <chrono>
`#include` <future>
+ `#include` <cstdlib>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <chrono> | |
| #include <future> | |
| #include <behaviortree_cpp/loggers/groot2_protocol.h> | |
| #include <behaviortree_cpp/loggers/groot2_publisher.h> | |
| #include <gtest/gtest.h> | |
| `#include` <chrono> | |
| `#include` <future> | |
| `#include` <cstdlib> | |
| `#include` <behaviortree_cpp/loggers/groot2_protocol.h> | |
| `#include` <behaviortree_cpp/loggers/groot2_publisher.h> | |
| `#include` <gtest/gtest.h> |
🤖 Prompt for AI Agents
In `@tests/gtest_groot2_publisher.cpp` around lines 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.
|
thread sanitizer looks unhappy 😃 https://github.com/BehaviorTree/BehaviorTree.CPP/actions/runs/21073716797/job/60609993659?pr=1058 |
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
✏️ Tip: You can customize this high-level summary in your review settings.