Fix topic statistics for IPC subscriptions#3130
Open
jayyoung wants to merge 1 commit intoros2:rollingfrom
Open
Fix topic statistics for IPC subscriptions#3130jayyoung wants to merge 1 commit intoros2:rollingfrom
jayyoung wants to merge 1 commit intoros2:rollingfrom
Conversation
If use_intra_process_comms is enabled, messages are delivered via SubscriptionIntraProcess::execute_impl() which previously never called the topic statistics handler, this causes all statistics to report NaN values. The fix here puts a type-erased StatsHandlerFn in SubscriptionIntraProcess and wires it up from the Subscription via a lambda. Using std::function avoids a circular include chain that would occur if subscription_topic_statistics.hpp were included directly from subscription_intra_process.hpp via publisher.hpp/callback_group.hpp. The source_timestamp is set to the receive time so that message_age reports 0ms rather than an un-initialised value. IPC delivery has little/no expected transport latency, by definition so near-zero age is expected. Fixes ros2#2911 Signed-off-by: jayyoung <jay.young@gmail.com>
jayyoung
force-pushed
the
fix-topic-stats-when-using-ipc
branch
from
ce94613 to
7df6693
Compare
tonynajjar
reviewed
Apr 15, 2026
| msg_info.publisher_gid = {0, {0}}; | ||
| msg_info.from_intra_process = true; | ||
|
|
||
| std::chrono::time_point<std::chrono::system_clock> now; |
Contributor
There was a problem hiding this comment.
I propose to declare now in the scope of the if below since it's only used there. Secondly I propose to declare nanos outside so that you can reuse it later without casting, since now doesn't change
tonynajjar
reviewed
Apr 15, 2026
| #include <stdexcept> | ||
| #include <string> | ||
| #include <type_traits> | ||
| #include <utility> |
Contributor
|
Could you also add a test that verifies that with IPC enabled the stats handler fires and reports a non-nan age? |
Contributor
|
@fujitatomoya we'd love to get this merged in before the freeze of rclcpp, I believe on the 20th April. Anything else you would like modified? |
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
2 participants
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.
Continuing on from the discussion here on the original fix: #2913 (comment) which addresses #2911 we are opening this PR at @fujitatomoya request.
Description
The root issue: We discovered that when
use_intra_process_commsis enabled, messages are delivered viaSubscriptionIntraProcess::execute_impl()which previously never called the topic statistics handler. This causes all statistics to reportNaNvalues, making it impossible for us distinguish a healthy IPC subscription from an unhealthy one.This fix: Here we add a type-erased
StatsHandlerFnfunction toSubscriptionIntraProcessand connect it up from theSubscriptionvia a lambda. Using the lambda rather than a pointer toSubscriptionTopicStatisticsavoids a circular include chain we discovered that could occur ifsubscription_topic_statistics.hppwas included directly fromsubscription_intra_process.hpp.In the code, we set
source_timestampto the receive time so thatmessage_agereports0msrather than an un-initialised value. Our reasoning is that: IPC delivery has no or little transport latency, so near-zero age is OK and expected. As discussed in the original fix PR, there is a one-time warning emit to inform users thatmessage_ageis not meaningful for IPC subscriptions.IN summary this PR is a revised implementation of the original PR #2913 by @roman-y-wu, with the following differences:
std::functiontype erasure to avoid the circular include issue we foundsource_timestampto prevent message_age reporting an invalid large value (uninitialised timestamp)From the original PR, we kept the one-time
RCLCPP_WARN_ONCElog message to ensures users are not silently surprised bymessage_agereading ~0ms when using IPC subscriptions.Fixes #2911
Is this user-facing behavior change?
Yeah it is for us, and addresses an issue we find in production. With this fix topic statistics now report valid values for subscriptions using IPC. Previously all statistics were NaN when IPC was enabled, and this behaviour bit us.
Did you use Generative AI?
Not on our side. This PR is a small adaptation of a older fix that already existed: #2913
Additional Information
This fix was first validated as a backport onto ROS 2 Kilted (
rclcpp==29.5.6) and has been running in production at Dexory across a fleet of 126 autonomous warehouse robots for over one month with no issues. Our robots use IPC-enabled lidar pipelines and rely on topic stats to monitor sensor health.message_periodstatistics are now correct and our monitoring correctly detects legitimate sensor failures with this fix.