Skip to content

fix(rmw_event): reject unsupported event types in *_event_init - #61

Merged
benaliabderrahmane merged 1 commit into
develfrom
fix/event-init-reject-unsupported
Sep 8, 2026
Merged

fix(rmw_event): reject unsupported event types in *_event_init#61
benaliabderrahmane merged 1 commit into
develfrom
fix/event-init-reject-unsupported

Conversation

@benaliabderrahmane

@benaliabderrahmane benaliabderrahmane commented Aug 19, 2026

Copy link
Copy Markdown
Owner

Ports the July EventsExecutor crash fix to devel — it was committed on fix/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_socket run of the ros2-benchmark-container suite on devel e819eb5):

terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  failed to set the on new message callback for Event: error not set

Every subscription crashes under EventsExecutor (irobot_benchmark's default). *_event_init returned RMW_RET_OK for every event type while rmw_event_type_is_supported() returns false and rmw_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_init functions (RMW_SET_ERROR_MSG + RMW_RET_UNSUPPORTED), which rclcpp handles as the expected UnsupportedEventTypeException. Adds test_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.

@benaliabderrahmane
fix(rmw_event): reject unsupported event types in *_event_init
7e39aa3 
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.
@benaliabderrahmane benaliabderrahmane mentioned this pull request Aug 19, 2026
Closed
benaliabderrahmane
benaliabderrahmane commented Sep 8, 2026
View reviewed changes

@benaliabderrahmane benaliabderrahmane left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@benaliabderrahmane
benaliabderrahmane merged commit 04d16f2 into devel Sep 8, 2026
3 checks passed
This was referenced Sep 8, 2026
Merged
Merged
Merged
benaliabderrahmane added a commit that referenced this pull request Sep 17, 2026
@benaliabderrahmane @claude
test(rmw_event): cover the new identifier guard, iterate every type o…
7e91404 
…n every distro

- SetOnNewMessageCallbackRejectsForeignSubscription pins the identifier
  check this PR adds to rmw_subscription_set_on_new_message_callback; it
  was the one guarded entry point without a test.
- The *RejectsEveryEventType loops were bounded by RMW_EVENT_INVALID, which
  is the last enumerator on Jazzy but 0 from Kilted on, so they ran a single
  iteration on three of the four CI distros. Bound them by whichever of it
  and RMW_EVENT_PUBLICATION_MATCHED is larger.
- Drop the claim that rcl reuses the event_info buffer across calls; no
  upstream layer does. The rule it was propping up stands on its own.
- CHANGELOG: state the *_event_init refusal the entry is headlined with
  (#61), and stop claiming every other entry point already had the
  identifier check - only the service and client callback setters did.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NUNQNo26cKRPrVXcaHZnje
benaliabderrahmane added a commit that referenced this pull request Sep 18, 2026
@benaliabderrahmane @claude
test(rmw_event): cover the new identifier guard, iterate every type o…
cb8abd7 
…n every distro

- SetOnNewMessageCallbackRejectsForeignSubscription pins the identifier
  check this PR adds to rmw_subscription_set_on_new_message_callback; it
  was the one guarded entry point without a test.
- The *RejectsEveryEventType loops were bounded by RMW_EVENT_INVALID, which
  is the last enumerator on Jazzy but 0 from Kilted on, so they ran a single
  iteration on three of the four CI distros. Bound them by whichever of it
  and RMW_EVENT_PUBLICATION_MATCHED is larger.
- Drop the claim that rcl reuses the event_info buffer across calls; no
  upstream layer does. The rule it was propping up stands on its own.
- CHANGELOG: state the *_event_init refusal the entry is headlined with
  (#61), and stop claiming every other entry point already had the
  identifier check - only the service and client callback setters did.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NUNQNo26cKRPrVXcaHZnje
benaliabderrahmane added a commit that referenced this pull request Oct 2, 2026
@benaliabderrahmane @claude
test(rmw_event): cover the new identifier guard, iterate every type o…
3550196 
…n every distro

- SetOnNewMessageCallbackRejectsForeignSubscription pins the identifier
  check this PR adds to rmw_subscription_set_on_new_message_callback; it
  was the one guarded entry point without a test.
- The *RejectsEveryEventType loops were bounded by RMW_EVENT_INVALID, which
  is the last enumerator on Jazzy but 0 from Kilted on, so they ran a single
  iteration on three of the four CI distros. Bound them by whichever of it
  and RMW_EVENT_PUBLICATION_MATCHED is larger.
- Drop the claim that rcl reuses the event_info buffer across calls; no
  upstream layer does. The rule it was propping up stands on its own.
- CHANGELOG: state the *_event_init refusal the entry is headlined with
  (#61), and stop claiming every other entry point already had the
  identifier check - only the service and client callback setters did.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NUNQNo26cKRPrVXcaHZnje
benaliabderrahmane added a commit that referenced this pull request Oct 5, 2026
@benaliabderrahmane Abderahmane BENALI @claude
fix(rmw_event): validate handles and set an error message on the fail…
ba97253 
…ure paths (#64)

* fix(rmw_event): validate handles and set an error message on the failure paths

rmw_take_event, rmw_event_set_callback, both *_event_init functions and
rmw_subscription_set_on_new_message_callback now reject a NULL handle with
RMW_RET_INVALID_ARGUMENT and a foreign one with
RMW_RET_INCORRECT_RMW_IMPLEMENTATION, and every failure path leaves an
error message - the missing message was the ': error not set' half of the
original EventsExecutor crash report.

rmw_event_fini deliberately validates nothing: rcl_event_fini skips the
call when impl is NULL, which is exactly what a rejected init leaves
behind, so guards there defended a state the stack cannot produce. Nothing
to release either - rmw_event_t::data aliases the endpoint's impl struct.

Squashed from:
- fix(rmw_event): validate handles and set an error message on the failure paths
- fix(rmw_event): drop the unreachable rmw_event_fini guards, check the identifier in set_callback

* test(rmw_event): cover the new identifier guard, iterate every type on every distro

- SetOnNewMessageCallbackRejectsForeignSubscription pins the identifier
  check this PR adds to rmw_subscription_set_on_new_message_callback; it
  was the one guarded entry point without a test.
- The *RejectsEveryEventType loops were bounded by RMW_EVENT_INVALID, which
  is the last enumerator on Jazzy but 0 from Kilted on, so they ran a single
  iteration on three of the four CI distros. Bound them by whichever of it
  and RMW_EVENT_PUBLICATION_MATCHED is larger.
- Drop the claim that rcl reuses the event_info buffer across calls; no
  upstream layer does. The rule it was propping up stands on its own.
- CHANGELOG: state the *_event_init refusal the entry is headlined with
  (#61), and stop claiming every other entry point already had the
  identifier check - only the service and client callback setters did.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NUNQNo26cKRPrVXcaHZnje

---------

Co-authored-by: Abderahmane BENALI <abderahmane.benali@exail.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@benaliabderrahmane