Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add statistics for handle_loaned_message #1927

Conversation

Barry-Xu-2018
Copy link
Collaborator

Add statistics for handle_loaned_message().
If loan message is enabled, some tests in test_subscription_topic_statistics fail. This issue is found in ros2/rmw_fastrtps#568 (comment)

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Need to resolve the question about double delivery and the interaction with intra-process.

@Barry-Xu-2018
Copy link
Collaborator Author

Barry-Xu-2018 commented May 16, 2022

(Edit: Progress -> Process)

@fujitatomoya @wjwwood

This is a case of double delivery.
The topic is '/number' for Pub1/Pub2 and Sub1/Sub2.
Message is Int32 (Pod)
RMW: FastDDS (with ros2/rmw_fastrtps#568 )

  • Process A
    • Pub1 (use_intra_process)
    • Sub 1 (use_intra_process)
  • Process B
    • Pub2

At this time, Sub1 receives messages from Pub1 via dispatch_intra_process(). Sub1 receives messages from Pub2 via handle_loaned_message().

Now start a new subscriber

  • Process C
    • Sub2

In order to send message to Sub2, Pub1 will send message via RMW.
At this time, for the message from Pub1, Sub1 can receive messages via handle_loaned_message() and via dispatch_intra_process().
This is double delivery.

So below code is necessary for handle_loaned_message()

    if (matches_any_intra_process_publishers(&message_info.get_rmw_message_info().publisher_gid)) {
      // In this case, the message will be delivered via intra process and
      // we should ignore this copy of the message.
      return;
    }

Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
This reverts commit 565d683.

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-statistics-for-handle-loaned-message branch from 565d683 to 8a70502 Compare May 16, 2022 07:41
@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018

thanks for checking!

At this time, Sub1 receives messages from Pub1 via dispatch_intra_process().

  • Process A
    • Pub1 (use_intra_process, cannot use LoanedMessage)
    • Sub1 (use_intra_process)

in this situation, How Sub1 avoid to call handle_loaned_message?

my expectation is, Sub1 recognizes can_loan_messages is true since data type is PoD. So Sub1 would call handle_loaned_message instead of handle_message? currently (w/o this PR), handle_loaned_message does not call matches_any_intra_process_publishers, so i was thinking that there might be double delivery.

i mean there seems to be a double delivery problem in intra-process communication.

@Barry-Xu-2018
Copy link
Collaborator Author

@fujitatomoya

my expectation is, Sub1 recognizes can_loan_messages is true since data type is PoD. So Sub1 would call handle_loaned_message instead of handle_message? currently (w/o this PR), handle_loaned_message does not call matches_any_intra_process_publishers, so i was thinking that there might be double delivery.

Right.
I forgot to mention ros2/rmw_fastrtps#568 is applied in my test environment. So even if data-sharing is OFF, can_loan_message is true once message is PoD.
If not apply ros2/rmw_fastrtps#568, handle_message is called instead of handle_loaned_message.

@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018 yeah, that means we have an unexpected double delivery problem if we enable data sharing for rmw_fastfrpt with current code.

anyway, solution can be applied on this PR.

@fujitatomoya
Copy link
Collaborator

we need to backport this to humble, galactic and foxy.

@fujitatomoya
Copy link
Collaborator

@wjwwood @ivanpauno could you take a look when you have time? I will start the CI.

@fujitatomoya
Copy link
Collaborator

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya fujitatomoya merged commit 5c68830 into ros2:master May 19, 2022
@fujitatomoya fujitatomoya added the bug Something isn't working label May 19, 2022
@fujitatomoya
Copy link
Collaborator

@mergify backport humble galactic foxy

mergify bot pushed a commit that referenced this pull request May 19, 2022
* Add statistics for handle_loaned_message

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 5c68830)
mergify bot pushed a commit that referenced this pull request May 19, 2022
* Add statistics for handle_loaned_message

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 5c68830)

# Conflicts:
#	rclcpp/include/rclcpp/subscription.hpp
mergify bot pushed a commit that referenced this pull request May 19, 2022
* Add statistics for handle_loaned_message

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 5c68830)

# Conflicts:
#	rclcpp/include/rclcpp/subscription.hpp
@mergify
Copy link

mergify bot commented May 19, 2022

backport humble galactic foxy

✅ Backports have been created

fujitatomoya pushed a commit that referenced this pull request Jun 9, 2022
* Add statistics for handle_loaned_message

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 5c68830)

Co-authored-by: Barry Xu <barry.xu@sony.com>
fujitatomoya pushed a commit that referenced this pull request Jun 9, 2022
* Add statistics for handle_loaned_message (#1927)

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 5c68830)

# Conflicts:
#	rclcpp/include/rclcpp/subscription.hpp

* Fix merge conflicts (#1939)

Signed-off-by: Barry Xu <barry.xu@sony.com>
fujitatomoya pushed a commit that referenced this pull request Jun 9, 2022
* Add statistics for handle_loaned_message (#1927)

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 5c68830)

* Fix merge conflicts (#1938)

Signed-off-by: Barry Xu <barry.xu@sony.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants