fix(rmw_event): reject unsupported event types in *_event_init - #61
Merged
Merged
Conversation
rmw_publisher_event_init/rmw_subscription_event_init always returned RMW_RET_OK regardless of event_type, while rmw_event_type_is_supported() always returns false and rmw_event_set_callback() always returns RMW_RET_UNSUPPORTED without setting an error message. rclcpp's EventHandler constructor treats RMW_RET_UNSUPPORTED from *_event_init specially (UnsupportedEventTypeException, swallowed by SubscriptionBase::bind_event_callbacks for the default incompatible-QoS/ type callbacks every subscription gets). Since init always "succeeded", rclcpp built a live event handler for an event we don't actually support, and the failure only surfaced later, unhandled, when the EventsExecutor tried to register its callback - crashing with "failed to set the on new message callback for Event: error not set" on every subscription, under EventsExecutor specifically. Fix: check rmw_event_type_is_supported() in both *_event_init functions before reporting success, matching the RMW_SET_ERROR_MSG + UNSUPPORTED pattern already used elsewhere in this file (e.g. rmw_subscription_set_content_filter). Adds test/test_rmw_event.cpp, confirmed to fail on the old behavior and pass after the fix. Full existing suite (128 tests) still passes. Found while verifying rmw_unix_socket_cpp against ros2-benchmark-container and performance_test: EventsExecutor (default in both) crashed on every pub/sub topology; SingleThreadedExecutor/MultiThreadedExecutor were unaffected.
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
1 participant
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.
Ports the July EventsExecutor crash fix to
devel— it was committed onfix/transient-local-late-joiner-replay(3d246ed) but that branch's feature was superseded by #60 and this fix never landed with it.Symptom (reproduced today, first
unix_socketrun of the ros2-benchmark-container suite on devele819eb5):Every subscription crashes under EventsExecutor (irobot_benchmark's default).
*_event_initreturnedRMW_RET_OKfor every event type whilermw_event_type_is_supported()returns false andrmw_event_set_callback()returns UNSUPPORTED without an error message — so rclcpp built a live handler for the default incompatible-QoS event and the failure surfaced later, unhandled, at callback registration.Fix: check
rmw_event_type_is_supported()in both*_event_initfunctions (RMW_SET_ERROR_MSG +RMW_RET_UNSUPPORTED), which rclcpp handles as the expectedUnsupportedEventTypeException. Addstest_rmw_event.cpp(verified fail-then-pass in July). 15/15 ctest green on this branch.Clean cherry-pick of
3d246ed; no adaptation was needed. The benchmark image is pinned to this commit until it merges.