Shutdown callback for service interfaces#31
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a ChangesShutdown Callback Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@ClemensElflein Is it fine to use std::function for service interfaces? We don't seem to have portables there. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libxbot-service-interface/include/xbot-service-interface/XbotServiceInterface.hpp (1)
20-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the
Start()Doxygen block adjacent toStart().The new
ShutdownCallbackalias now sits between theStart()documentation and theStart()declaration, so generated docs will attach theStart()text to the alias instead.📚 Proposed documentation layout
-/** - * Call this method to start xbot_framework. - * - * `@param` register_signal_handlers true to handle signals (CTRL+C), false to - * manually stop using Stop() - * `@return` The context - */ using ShutdownCallback = std::function<void()>; /** * Register a callback invoked after Stop() when SIGINT/SIGTERM is received. * Must be called before Start() if register_signal_handlers is true. */ void SetShutdownCallback(ShutdownCallback callback); +/** + * Call this method to start xbot_framework. + * + * `@param` register_signal_handlers true to handle signals (CTRL+C), false to + * manually stop using Stop() + * `@return` The context + */ Context Start(bool register_signal_handlers = true, std::string bind_ip = "0.0.0.0", bool start_rest_api = false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libxbot-service-interface/include/xbot-service-interface/XbotServiceInterface.hpp` around lines 20 - 35, The Doxygen documentation block for the Start() method is not adjacent to its declaration due to the ShutdownCallback type alias and SetShutdownCallback() method being placed between them. This causes the documentation to attach to the wrong entity. Move the ShutdownCallback type alias definition and the SetShutdownCallback() method declaration to appear before the Start() Doxygen comment block, so that the documentation comment for Start() is directly adjacent to the Start() method declaration.
🧹 Nitpick comments (1)
libxbot-service-interface/src/XbotServiceInterface.cpp (1)
38-39: ⚡ Quick winInclude
<utility>forstd::move.Line 39 uses
std::move, but this file does not include<utility>directly; relying on transitive includes is fragile across toolchains.🧩 Proposed include fix
`#include` <csignal> `#include` <mutex> +#include <utility> `#include` <xbot-service-interface/XbotServiceInterface.hpp>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libxbot-service-interface/src/XbotServiceInterface.cpp` around lines 38 - 39, The SetShutdownCallback function uses std::move to move the callback parameter, but the file does not directly include the <utility> header where std::move is defined. Add `#include` <utility> to the includes section at the top of XbotServiceInterface.cpp to ensure std::move is properly declared, avoiding reliance on fragile transitive includes that may break across different toolchains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libxbot-service-interface/src/XbotServiceInterface.cpp`:
- Around line 31-39: The SignalHandler function violates async-signal-safety by
directly calling Stop() (which uses logging, mutexes, and virtual calls) and
invoking shutdown_callback (arbitrary C++ code). Additionally, shutdown_callback
is accessed without synchronization between the read in SignalHandler and
mutation in SetShutdownCallback, creating a data race. Refactor to defer signal
handling: use sigwait() on a dedicated thread or implement a self-pipe/eventfd
mechanism to handle the actual Stop() call and callback invocation from a normal
(non-signal) execution context with proper synchronization. Also add `#include`
<utility> at the top of the file since std::move is used in SetShutdownCallback
without it.
---
Outside diff comments:
In
`@libxbot-service-interface/include/xbot-service-interface/XbotServiceInterface.hpp`:
- Around line 20-35: The Doxygen documentation block for the Start() method is
not adjacent to its declaration due to the ShutdownCallback type alias and
SetShutdownCallback() method being placed between them. This causes the
documentation to attach to the wrong entity. Move the ShutdownCallback type
alias definition and the SetShutdownCallback() method declaration to appear
before the Start() Doxygen comment block, so that the documentation comment for
Start() is directly adjacent to the Start() method declaration.
---
Nitpick comments:
In `@libxbot-service-interface/src/XbotServiceInterface.cpp`:
- Around line 38-39: The SetShutdownCallback function uses std::move to move the
callback parameter, but the file does not directly include the <utility> header
where std::move is defined. Add `#include` <utility> to the includes section at
the top of XbotServiceInterface.cpp to ensure std::move is properly declared,
avoiding reliance on fragile transitive includes that may break across different
toolchains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 172955a2-f1fc-4cad-96e2-fa3c19e86484
📒 Files selected for processing (2)
libxbot-service-interface/include/xbot-service-interface/XbotServiceInterface.hpplibxbot-service-interface/src/XbotServiceInterface.cpp
|
Needed for ClemensElflein/open_mower_ros#304. |
rovo89
force-pushed
the
feat/shutdown-callback
branch
from
a1e4529 to
54a0154
Compare
Yes, the interface side is linux only, so we can go fancy 👍 |
|
LGTM, thank you for the PR! |
ClemensElflein
force-pushed
the
feat/shutdown-callback
branch
from
54a0154 to
85ed2b4
Compare
2 participants
Summary by CodeRabbit