fix(agnocastlib): fix error handling in executor#1139
Draft
Koichi98 wants to merge 6 commits intofix/error_handling_agnocastlibfrom
Draft
fix(agnocastlib): fix error handling in executor#1139Koichi98 wants to merge 6 commits intofix/error_handling_agnocastlibfrom
Koichi98 wants to merge 6 commits intofix/error_handling_agnocastlibfrom
Conversation
…into fix/error_handling_executor
Signed-off-by: Koichi Imai <koichi.imai.2@tier4.jp>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates agnocastlib’s executor/signal/epoll error handling to avoid exit(EXIT_FAILURE) and instead surface failures via C++ exceptions, enabling callers to decide how to handle fatal vs. recoverable errors.
Changes:
- Replace
RCLCPP_ERROR+exit()(and associatedclose()calls) withstd::runtime_error/std::logic_error/std::invalid_argumentacross executors and epoll helpers. - Propagate epoll/signal installation failures via exceptions, and standardize error messages for these failures.
- Keep multi-threaded executors resilient by catching exceptions in worker threads and rethrowing a summarized failure in the caller thread.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/agnocastlib/src/node/agnocast_signal_handler.cpp | Switch SIGINT/SIGTERM installation failures from process-exit to throwing exceptions. |
| src/agnocastlib/src/node/agnocast_only_single_threaded_executor.cpp | Throw on illegal re-entrant spin() instead of exiting. |
| src/agnocastlib/src/node/agnocast_only_multi_threaded_executor.cpp | Throw on illegal re-entrant spin(), keep worker-thread exception handling. |
| src/agnocastlib/src/node/agnocast_only_executor.cpp | Convert constructor and node/group association fatal paths to exceptions. |
| src/agnocastlib/src/node/agnocast_only_callback_isolated_executor.cpp | Convert fatal add-node/spin guard paths to exceptions. |
| src/agnocastlib/src/agnocast_single_threaded_executor.cpp | Convert nullptr callback group + double-spin fatal paths to exceptions. |
| src/agnocastlib/src/agnocast_multi_threaded_executor.cpp | Convert validation + double-spin fatal paths to exceptions; keep worker-thread catch/flagging. |
| src/agnocastlib/src/agnocast_executor.cpp | Throw on epoll_create1 failure instead of exiting. |
| src/agnocastlib/src/agnocast_epoll.cpp | Throw on epoll/mq_receive/internal-map errors instead of exiting. |
| src/agnocastlib/src/agnocast_callback_isolated_executor.cpp | Convert double-spin and duplicate add/remove error paths to exceptions. |
| src/agnocastlib/include/agnocast/agnocast_epoll.hpp | Convert epoll registration failures in prepare_epoll_impl to exceptions. |
Comments suppressed due to low confidence (1)
src/agnocastlib/src/node/agnocast_only_executor.cpp:47
- AgnocastOnlyExecutor now throws on failures, and SignalHandler::install() can also throw. If install()/register_shutdown_event() throws here, the constructor will unwind and the destructor will not run, leaking epoll_fd_ and shutdown_event_fd_ (and leaving shutdown_event_fd_ registered if registration happened). Wrap the SignalHandler calls in a try/catch that closes/unregisters the fds before rethrowing, or use RAII wrappers for these fds during construction.
if (epoll_ctl(epoll_fd_, EPOLL_CTL_ADD, shutdown_event_fd_, &ev) == -1) {
close(shutdown_event_fd_);
close(epoll_fd_);
throw std::runtime_error(
std::string("epoll_ctl for shutdown_event_fd failed: ") + strerror(errno));
}
SignalHandler::install();
SignalHandler::register_shutdown_event(shutdown_event_fd_);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Koichi <koichi.imai.2@tier4.jp>
…st into fix/error_handling_executor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related links
How was this PR tested?
bash scripts/test/e2e_test_1to1(required)bash scripts/test/e2e_test_2to2(required)Notes for reviewers
Version Update Label (Required)
Please add exactly one of the following labels to this PR:
need-major-update: User API breaking changesneed-minor-update: Internal API breaking changes (heaphook/kmod/agnocastlib compatibility)need-patch-update: Bug fixes and other changesImportant notes:
need-major-updateorneed-minor-update, please include this in the PR title as well.fix(foo)[needs major version update]: barorfeat(baz)[needs minor version update]: quxrun-build-testlabel. The PR can only be merged after the build tests pass.See CONTRIBUTING.md for detailed versioning rules.