Maybe definition of using DataSharingKind is not already needed like in file publisher.cpp.

CI: (only rmw_fastrtps)

• Linux
• Linux-aarch64
• macOS
• Windows

@fujitatomoya Would you mind triggering CI again on this PR?

CI:

• Linux
• Linux-aarch64
• Windows

@MiguelCompany still unstable, could you check the log?

@MiguelCompany friendly ping.

@fujitatomoya I've been investigating the failures on rclcpp test_subscription_topic_statistics, and I've found the issue.

When using loans, method handle_loaned_message is called on the subscription instead of handle_message. See here
But class SubscriptionTopicStatistics only has an override for handle_message here

We need to add an override for handle_loaned_message on SubscriptionTopicStatistics, so statistics are also accounted for loaned messages.

CC: @Barry-Xu-2018

@Barry-Xu-2018 do we have any progress on this?

@fujitatomoya

I don't try it since this PR isn't merged.
#579 depends on this. So you want to check it now.

We need to add an override for handle_loaned_message on SubscriptionTopicStatistics, so statistics are also accounted for loaned messages.

i think this needs to be addressed with this PR at the same time. otherwise, we cannot have green light on CI.

@fujitatomoya

I try this PR and confirm message (pod) can be loaned without data-sharing enabled.

About the failures on rclcpp test_subscription_topic_statistics

We need to add an override for handle_loaned_message on SubscriptionTopicStatistics, so statistics are also accounted for loaned messages.

I have a different opinion. @MiguelCompany

For handle_message(), it has codes to get topic statistics (call handle_message() of SubscriptionTopicStatistics).
https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/subscription.hpp#L345-L349

But for handle_loaned_message(), nothing is done for statistics even if statistics is enabled. So test failed.
Therefore, we can add codes to get topic statistics, like codes in handle_message().

https://github.com/ros2/rclcpp/blob/d8e1aed520047b5da28d65cbfbdf781358c14fc6/rclcpp/include/rclcpp/subscription.hpp#L361-L371

@Barry-Xu-2018 I think you are right.

On my first look at the code, I thought that SubscriptionTopicStatistics was a child class of Subscription, but now I see I was wrong.

The point is what you say. Statistics should be handled when handling a loaned message.

As an extra fix, I think we should also mimic the rest of the behavior on handle_message (i.e. ignore rclcpp intraprocess processing): https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/subscription.hpp#L329-L333

@Barry-Xu-2018 ah, right what we need to do is modify Subscription::handle_loaned_message. could you work on this? i am happy to do review.

could you work on this? i am happy to do review.

Okay. I make it.

@fujitatomoya

I created a PR ros2/rclcpp#1927.
Please help to review.

CI:

• Linux
• Linux-aarch64
• Windows

i thinik repo file is old, need to be rebased.

CI withhttps://gist.githubusercontent.com/fujitatomoya/f93a8b37c13378d829aaddcc24bf72f2/raw/89f243a68e8aef6d8dc92e306007e99350c00942/ros2.repos

• Linux
• Linux-aarch64
• Windows

i thinik repo file is old, need to be rebased.

I can rebase the branch if necessary

@fujitatomoya I rebased this one.

@MiguelCompany thanks!

CI:

• Linux
• Linux-aarch64
• Windows

@fujitatomoya I think the failing tests are wrong, as this line should be with std::launch::async instead of std::launch::deferred, as it is done on the test above.

CI with ros2/rclcpp#1940

• Linux
• Linux-aarch64
• Windows

CI:

• Linux
• Linux-aarch64
• Windows

I will run whole CI for this, since this could affect many packages.

CI:

• Linux
• Linux-aarch64
• Windows

Note:

after this PR is merged, rmw_fastrtps tries to enable LoanedMessage to borrow the memory from Fast-DDS as long as message type is bounded (PoD) by default. Application is still responsible to use LoanedMessage Class to use middleware memory for zero-copy inter-process transport.

see also, #596 (comment)

@fujitatomoya Is there anything pending here?

I just need to rerun the CI, thanks for the ping.

CI (Full with rmw_fastrtps static):

• Linux
• Linux-aarch64
• Windows