fix(rmw_event): validate handles and set an error message on the failure paths - #64
Open
benaliabderrahmane wants to merge 2 commits into
Open
fix(rmw_event): validate handles and set an error message on the failure paths#64benaliabderrahmane wants to merge 2 commits into
benaliabderrahmane wants to merge 2 commits into
Conversation
…ure paths PR #61 made *_event_init reject unsupported event types, which is the only refusal rclcpp swallows. The remaining event entry points were still bare stubs, and two of the gaps stay reachable. rmw_event_set_callback returned RMW_RET_UNSUPPORTED without setting an error message. That missing message is the ": error not set" half of the original EventsExecutor crash report ("failed to set the on new message callback for Event: error not set"), which is what made the failure hard to place. rclcpp can no longer reach this function now that init rejects every type, but a failure code with nothing behind it is a dead end for anyone calling it directly. rmw_take_event and rmw_event_fini validated neither their handle nor its implementation identifier, unlike every other entry point here. rmw_event_fini needs one allowance: rclcpp's EventHandler destructor runs even when its constructor bailed out on the RMW_RET_UNSUPPORTED from *_event_init, so it is handed a zero-initialized handle. Finalizing that is not a caller error, and reporting one would turn every swallowed UnsupportedEventTypeException into a spurious failure on the way out. Also adds the RMW_CHECK_TYPE_IDENTIFIERS_MATCH that both *_event_init and rmw_subscription_set_on_new_message_callback were missing; the service and client callback setters already had it. The tests iterate the whole rmw_event_type_t enum rather than naming event types. It has no explicit initializers, so both the ordinals and RMW_EVENT_INVALID shift as upstream adds event types, and this builds against four distros. Four of the ten tests fail on the previous behavior and pass after this change. Full suite green on Jazzy: 164 tests, 0 failures.
benaliabderrahmane
added a commit
that referenced
this pull request
Sep 9, 2026
's entry Four corrections to the docs this PR adds. "Edge-driven" appeared three times for the listener. The intent was event-driven rather than timer-driven, which is what the surrounding sentences argue, but the term means the opposite of what the code does: listener_watch registers EPOLLIN without EPOLLET and relies on level-triggering to notice datagrams that arrived before the watch. Now "event-driven, not timer-driven". The delivery_fd section described one eventfd per context and claimed "there is no order in which it is missed". That only held for whichever wait set woke first - a read drains the whole eventfd counter - so the section now explains why the fd belongs to the wait set, and the deregister-before-close rule that comes with it. The lifetime rule understated itself. listener_mutex is one per context, so a callback must not destroy, register on, or clear the callback of any endpoint in the context, nor create or destroy a wait set - not just its own endpoint - and listener_unwatch blocks on whatever drain is in flight, not one of "that endpoint". The CHANGELOG documented #64's event-API changes, and #64 is not an ancestor of this stack, so merging this without it shipped a changelog for absent code. That entry moves to #64, which now carries its own Unreleased section. Replaced here by the fixes this stack actually makes: the double backlog flush, the setters' error paths, and the std::thread throw. test/README.md gains rows for the four new tests and updates the two that were renamed. Docs only. Full suite green on Jazzy: 174 tests, 0 failures.
… identifier in set_callback Two follow-ups on this PR's own stated goal. rmw_event_fini's allowance for a zero-initialized handle defends a state the stack cannot produce. rcl_event_fini skips the call entirely when event->impl is NULL, and that is exactly what rcl_*_event_init leaves behind: it deallocates impl and nulls it before returning our RMW_RET_UNSUPPORTED. So no rejected init ever reaches this function, and neither does a foreign handle. Both guards go, and the function is a documented no-op again - CLAUDE.md's "no error handling for impossible scenarios". rmw_event_set_callback was left as the only event entry point without RMW_CHECK_TYPE_IDENTIFIERS_MATCH, which is the gap this PR set out to close. Added. The two rmw_event_fini tests collapse into one that pins the no-op contract; EventSetCallbackReportsUnsupportedWithAMessage gains the foreign-handle case, which fails before this change. Full suite green on Jazzy: 163 tests, 0 failures.
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.
Description
Finishes the event API surface that #61 started. #61 made
*_event_initrejectunsupported event types, which is the refusal
rclcpphandles cleanly. Theremaining entry points were still bare stubs, and two of the gaps stay reachable:
rmw_event_set_callbackreturnedRMW_RET_UNSUPPORTEDwithout setting anerror message. That missing message is the
": error not set"half of theoriginal
EventsExecutorcrash report, and it is what made the failure hard toplace.
rclcppcan no longer reach this function, but a failure code withnothing behind it is a dead end for anyone calling it directly.
rmw_take_eventvalidated neither its handle nor its implementationidentifier, unlike the endpoint and node entry points elsewhere in this RMW.
RMW_CHECK_TYPE_IDENTIFIERS_MATCHnow guards five entry points that weremissing it: both
*_event_initfunctions,rmw_take_event,rmw_event_set_callbackandrmw_subscription_set_on_new_message_callback; theservice and client callback setters already had it.
One deliberate omission worth a look during review:
rmw_event_finiis leftvalidating nothing at all — a bare
(void)event; return RMW_RET_OK;. An earlierrevision of this branch did guard it, and the guards came back out:
rcl_event_finiskips the call entirely when
event->implis NULL, and that is exactly whatrcl_*_event_initleaves behind, since it deallocatesimpland nulls it beforereturning our
RMW_RET_UNSUPPORTED. So no rejected init ever reaches thisfunction, and neither does a foreign handle — the guards were defending a state
the stack cannot produce, which is CLAUDE.md's "no error handling for impossible
scenarios". There is nothing to release either:
rmw_event_t::dataaliases theendpoint's impl struct, owned by
rmw_destroy_publisher/rmw_destroy_subscription. The function carries a comment saying so.Also adds an
## [Unreleased]/### Fixedsection toCHANGELOG.mdcoveringthe above.
Is this user-facing behavior change?
Yes, in three ways, all narrow:
rmw_event_set_callbacknow leaves an error message, sorcllogssomething useful instead of
error not set. It also now returnsRMW_RET_INVALID_ARGUMENTfor a null handle andRMW_RET_INCORRECT_RMW_IMPLEMENTATIONfor a foreign one, wheredevelreturned
RMW_RET_UNSUPPORTEDfor both.rmw_take_eventreturnsRMW_RET_INVALID_ARGUMENT/RMW_RET_INCORRECT_RMW_IMPLEMENTATIONon misuse that previously returnedRMW_RET_OK, and both*_event_initfunctions returnRMW_RET_INCORRECT_RMW_IMPLEMENTATIONfor a foreign endpoint wheredevelfell through to the unsupported-type check and returned
RMW_RET_UNSUPPORTED.Only reachable by passing a null or a foreign handle.
rmw_subscription_set_on_new_message_callbackreturnsRMW_RET_INCORRECT_RMW_IMPLEMENTATIONfor a foreign subscription, wheredevelwould have cast that subscription'sdatatormw_uds::UdsSubscription *and locked itscallback_mutex.rmw_event_finiis unchanged: it still returnsRMW_RET_OKfor every handle.No event type becomes supported.
rmw_event_type_is_supportedstill returnsfalsefor all of them.How was this tested?
test/test_rmw_event.cppgrows from 2 tests to 9. Three of the nine fail onthe current
develbehavior and pass after this change:EventInitRejectsForeignEndpoints,TakeEventRejectsBadArguments,EventSetCallbackReportsUnsupportedWithAMessage. The remaining six alsopass on
devel— includingEventFiniIsANoOp, which pins the contractdevelalready had.The two existing tests are broadened to iterate
0 .. RMW_EVENT_INVALIDratherthan naming two event types by hand.
rmw_event_type_thas no explicitinitializers, so both the ordinals and
RMW_EVENT_INVALIDshift as upstream addsevent types — no ordinal is hardcoded anywhere.
Full suite green locally on jazzy: 163 tests, 0 failures. kilted, rolling and
lyrical are on CI.
Did you use Generative AI?
Additional Information
Independent of the other branches in this series — can merge in any order.