Fix transient local IPC publish

[jefferyyjhsu]

Signed-off-by: Jeffery Hsu jefferyyjhsu@gmail.com

Fix transient local publish when inter and intra process communications are both present described in #2704.
Add a new test case for transient local publish mixing communication types.

[alsora]

Thank you @jefferyyjhsu !
The PR looks good to me

CI

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[alsora]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@jefferyyjhsu thanks for fixing. it looks some errors came up with CI, could you also check those?

[jefferyyjhsu]

@fujitatomoya and @alsora
I need some input from you guys.

1. The new unit test case I added is unstable, I am wondering if it's because I need to call rclcpp::spin_some() in the test case, so inter_process new_message_callback will be executed. However, I don't quite understand why the test case don't fail constantly locally for me.
2. TestServiceIntrospection.service_introspection_nominal is failing for me constantly on my local machine. I am working in a ros:rolling docker container. I am not sure if I should worry about it as it seems unrelated to my change here.

/ros2_ws/src/rclcpp/test/rclcpp/test_service_introspection.cpp:138: Failure
Value of: client_gid_arr
Expected: is equal to { '\x1' (1), '\xF' (15), '\xEB' (235), '}' (125, 0x7D), 'S' (83, 0x53), '{' (123, 0x7B), 'O' (79, 0x4F), '\x8C' (140), '\0', '\0', '\0', '\0', '\0', '\0', '\x14' (20), '\x4' (4) }
  Actual: { '\x1' (1), '\xF' (15), '\xEB' (235), '}' (125, 0x7D), 'S' (83, 0x53), '{' (123, 0x7B), 'O' (79, 0x4F), '\x8C' (140), '\0', '\0', '\0', '\0', '\0', '\0', '\x15' (21), '\x3' (3) }

[fujitatomoya]

The new unit test case I added is unstable, I am wondering if it's because I need to call rclcpp::spin_some() in the test case, so inter_process new_message_callback will be executed. However, I don't quite understand why the test case don't fail constantly locally for me.

hm... it does not ring a bell for me. @alsora any thoughts?

TestServiceIntrospection.service_introspection_nominal is failing for me constantly on my local machine. I am working in a ros:rolling docker container. I am not sure if I should worry about it as it seems unrelated to my change here.

this should be unrelated.

because you are using ros:rolling container to test your PR, the following fix is not included in rmw_fastrtps package.

ros2/rmw_fastrtps@8fd6f3c, without it, the test should fail.

after all, to avoid this package dependencies, i suggest that full source build with your rclcpp package branch, https://docs.ros.org/en/rolling/Installation/Alternatives/Ubuntu-Development-Setup.html

[clalancette]

1. The new unit test case I added is unstable, I am wondering if it's because I need to call rclcpp::spin_some() in the test case, so inter_process new_message_callback will be executed. However, I don't quite understand why the test case don't fail constantly locally for me.

Yes, you definitely need to spin for things to work. My suggestion is that you create an executor and spin right after setting up the subscription callbacks (and make sure to use spin, not spin_some, as that has some other drawbacks).

[alsora]

Yes, as others have said, you will need to create an executor and spin.
The node that you are using is not automatically added to any executor.

To expand on what others have said, you can:

• create an executor and add the node to it
• create a thread and let the executor spin in this thread
• do a "busy loop" until called becomes true or a timeout occurs
• stop the executor and join the thread

The new unit test case I added is unstable,

I would expect this test to fail 100% of the times with the current code.

[fujitatomoya]

lgtm with green CI

[fujitatomoya]

Pulls: #2708
Gist: https://gist.githubusercontent.com/fujitatomoya/b3ddef54881873cfc618f5cbfa81ae61/raw/1cfaa305cde6a30dfa3013ae440a9b914f5b0942/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14986

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[SteveMacenski]

@jefferyyjhsu @fujitatomoya should this be backported to Jazzy? Seems important and I don't see any ABI/API issues

[fujitatomoya]

@Mergifyio backport jazzy

[mergify]

backport jazzy

✅ Backports have been created

• #2722 Fix transient local IPC publish (backport #2708) has been created for branch jazzy