Add transient local durability support to publisher and subscriptions when using intra-process communication

[jefferyyjhsu]

Signed-off-by: Jeffery Hsu jefferyyjhsu@gmail.com
This PR adds transient local durability support to publishers and subscriptions when intra-process communication is used.
An intra-process buffer already existed on the subscription side. The PR adds the same style buffer to the publisher side and other supporting functions allowing intra process manager to publish saved messages for late joining subscriptions.

[roncapat]

This might allow to revert those temporary fixes in TF2:
ros2/geometry2#572
ros2/geometry2#607

[alsora]

CI

• Linux
• Linux-aarch64
• Windows

[alsora]

New CI:

• Linux
• Linux-aarch64
• Windows

[alsora]

• Linux
• Linux-aarch64
• Windows

[alsora]

• Linux
• Linux-aarch64
• Windows

[alsora]

CI

• Linux
• Linux-aarch64
• Windows

[alsora]

@wjwwood @fujitatomoya can you take a look at this PR?
It looks good to me

[fujitatomoya]

@alsora i will try in next week.

[jefferyyjhsu]

@fujitatomoya and @wjwwood ,
I know it's ROScon this week, but are there any updates?
Thanks!

[fujitatomoya]

@jefferyyjhsu sorry i am really caught on ROSCon... at least i cannot allocate time for this week.

[alsora]

Any update on reviewing this?

[jefferyyjhsu]

@fujitatomoya @wjwwood
Kind bump!

[alsora]

Build:

• Linux
• Linux-aarch64
• Windows

[alsora]

• Linux
• Linux-aarch64
• Windows

[alsora]

re-running windows

• Windows

[jefferyyjhsu]

__@fujitatomoya @iuhilnehc-ynos ,
Do you guys know why Windows CI is unstable with this CMake warning?
It's from tinyxml_vendor, so I don't understand why it only affects CI builds for my branch.

Starting >>> rosidl_dynamic_typesupport_fastrtps
--- output: tinyxml_vendor
Not searching for unused variables given on the command line.
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.20348.
-- The C compiler identification is MSVC 19.29.30153.0
-- The CXX compiler identification is MSVC 19.29.30153.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ament_cmake: 2.3.2 (C:/ci/ws/install/share/ament_cmake/cmake)
-- Found Python3: C:/Python38/python.exe (found version "3.8.3") found components: Interpreter
-- Could NOT find TinyXML (missing: TinyXML_LIBRARY TinyXML_INCLUDE_DIR)
CMake Warning (dev) at C:/Program Files/CMake/share/cmake-3.24/Modules/ExternalProject.cmake:3074 (message):
The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
not set. The policy's OLD behavior will be used. When using a URL
download, the timestamps of extracted files should preferably be that of
the time of extraction, otherwise code that depends on the extracted
contents might not be rebuilt if the URL changes. The OLD behavior
preserves the timestamps from the archive instead, but this is usually not
what you want. Update your project to the NEW behavior or specify the
DOWNLOAD_EXTRACT_TIMESTAMP option with a value of true to avoid this
robustness issue.
Call Stack (most recent call first):
C:/Program Files/CMake/share/cmake-3.24/Modules/ExternalProject.cmake:4170 (_ep_add_download_command)
CMakeLists.txt:24 (ExternalProject_Add)
This warning is for project developers. Use -Wno-dev to suppress it.

[clalancette]

Do you guys know why Windows CI is unstable with this CMake warning?
It's from tinyxml_vendor, so I don't understand why it only affects CI builds for my branch.

The gist that is being used for CI is outdated; we've since dropped tinyxml_vendor, so the gist needs to be updated.

[alsora]

Full CI with updated gist

• Linux
• Linux-aarch64
• Windows

[alsora]

Merging with green CI and approvals

[thomasmoore-torc]

@jefferyyjhsu - I noticed that as part of this PR, the ability to specify the type stored within the intra-process buffer for both the publisher and subscription was added. Is there a specific use case in mind for that option? It seems like setting it will circumvent the logic to minimize the number of necessary copies which are made when publishing a message.

[alsora]

@thomasmoore-torc If I remember correctly, this was done to avoid having to duplicate those buffer classes.
I need to double check the code though.

In any case, there should be some unit-tests that verify that ipc is doing the expected number of copies.

[jefferyyjhsu]

@thomasmoore-torc,
I only added that ability to the publisher side mostly to mirror how it was already implemented on the subscription side and to use the same buffer class as on the subscription side.

rclcpp/rclcpp/include/rclcpp/subscription_options.hpp

Line 62 in 8230d15

IntraProcessBufferType intra_process_buffer_type = IntraProcessBufferType::CallbackDefault;

I agree; there is a chance that this can result in unnecessary copies, but the default is set to

rclcpp/rclcpp/include/rclcpp/publisher_options.hpp

Line 45 in 8230d15

IntraProcessBufferType intra_process_buffer_type = IntraProcessBufferType::SharedPtr;

to try to minimize user errors.

[thomasmoore-torc]

@jefferyyjhsu - thanks for the clarification. I had looked at when the option was added to the publisher options but not the subscriber options.

From what I can tell, setting the publisher buffer type to UniquePtr would cause every buffered message to be copied as the publisher always calls add_shared().

@alsora - similarly, setting the subscriber buffer type to a specific value that is different from the actual callback type used by the callback will also result in an unnecessary copy. For example, setting the subscriber option to UniquePtr with the callback type being SharedPtr will cause a copy to be made when adding to the buffer. If the subscriber option is SharedPtr and the callback type is UniquePtr, then a copy will be made when consuming from the buffer.

I'm having a difficult time trying to think of use cases for when the buffer type should be different from the callback type. I suppose one use case for setting the subscriber buffer type to SharedPtr and the callback to UniquePtr would be if the subscriber expects to drop messages (e.g. small buffer size and long processing time) and it wants to defer the copy until the non-dropped messages dequeued.

[SteveMacenski]

@jefferyyjhsu @alsora I'm not seeing late joining subscriptions receiving transient local messages in Nav2 after trying to migrate to use IPC from this pull request. I have a ticket in Nav2 where we're discussing this (ros-navigation/navigation2#4691), but the problem that I'm seeing is that my transient local publishers for the costmap:

  occ_pub_ = create_publisher<nav_msgs::msg::OccupancyGrid>(
    topic_name,
    rclcpp::QoS(rclcpp::KeepLast(1)).transient_local().reliable());

Are not providing the data to external processes like Rviz or ros2 topic echo. I am however seeing it publish to those external processes once if they're running at the same time as the initial publication, but if I try afterward, it never receives a new message. I haven't found a way yet to functionally test if late-joining subscriptions within the process are properly receiving it or not. I did notice though that the tests in this PR don't cover the (a) transient local IPC on and (b) subscription from another process case.

Edit: I found a way, the in-process late-joining transient local subscribers are receiving the messages. It seems like only an issue for external ones. I've shown this not working in three ways

• ROS 2 CLI: no message received ros2 topic echo /map --qos-durability transient_local
• Rviz2: no map is received unless rviz is up before the publication is sent
• Software: I showed by adding the follow block into a function that runs periodically that it receives the update when all composition nodes are in the same container (i.e. process). Once I move the map server publisher into another component container, it fails to work since it would involve inter-process comms like the CLI or Rviz2.

  rclcpp::QoS map_qos(10);  // initialize to default
  map_qos.transient_local();
  map_qos.reliable();
  map_qos.keep_last(1);
  auto node = node_.lock();
  auto map_sub = node->create_subscription<nav_msgs::msg::OccupancyGrid>(
    map_topic_, map_qos, [this](nav_msgs::msg::OccupancyGrid::SharedPtr ) {
                // Lambda function to handle the message
                RCLCPP_INFO(logger_, "Received new map");
            });
  rclcpp::Rate r(1);
  r.sleep();

I think this is enough for me to file a ticket as an actual bug and not an oddity of Nav2