ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
514 stars 410 forks source link

use custom allocator from publisher option. #2478

Open fujitatomoya opened 3 months ago

fujitatomoya commented 3 months ago

address https://github.com/ros2/rclcpp/issues/2477

alsora commented 3 months ago
fujitatomoya commented 3 months ago

@thomasmoore-torc windows failure https://ci.ros2.org/job/ci_windows/21402/testReport/junit/(root)/rclcpp/test_publisher_with_type_adapter_gtest_missing_result/ seems that it gets crashed on somewhere in test_large_message_unique.

https://github.com/ros2/rclcpp/blob/f7a7954ae74f1b92164cbc21900b6cf89c5b8e16/rclcpp/test/rclcpp/test_publisher_with_type_adapter.cpp#L367-L386

Do you have any clue for that?

fujitatomoya commented 3 months ago

I can not reproduce this failure with linux platform.

thomasmoore-torc commented 3 months ago

@fujitatomoya - I don't see anything obvious as to why this would fail on the Windows platform. However, I don't readily have access to a Windows-based development environment to try and debug.

Did the most recent build fail with the same issues after you forced pushed the additional changes? I haven't discovered how to find those build results and the Rpr__rclcpp__ubuntu_noble_amd64 build in the checks seems different.

fujitatomoya commented 3 months ago

Did the most recent build fail with the same issues after you forced pushed the additional changes?

failure really related to this PR, but we can give it a shot only with rclcpp test.

fujitatomoya commented 3 months ago

@thomasmoore-torc the same failure. I do not have windows either, i would like to ask some help on this verification. in the meantime, i will take a look what went wrong in the source code.

[ RUN      ] TestPublisher.test_large_message_unique
-- run_test.py: return code 3221226356
-- run_test.py: generate result file 'C:/ci/ws/build/rclcpp/test_results/rclcpp/test_publisher_with_type_adapter.gtest.xml' with failed test
-- run_test.py: verify result file 'C:/ci/ws/build/rclcpp/test_results/rclcpp/test_publisher_with_type_adapter.gtest.xml'
fujitatomoya commented 3 months ago

@clalancette @alsora any thoughts?

Crola1702 commented 2 months ago

I think the failing test @fujitatomoya is mentioning was caused by https://github.com/ros2/rclcpp/pull/2443 (check investigation).

It's a consistent issue in Windows Debug

fujitatomoya commented 2 months ago

@Crola1702 thanks for the info.

I got a question why CI https://github.com/ros2/rclcpp/pull/2443#issuecomment-1978752604 on https://github.com/ros2/rclcpp/pull/2443 could not detect this warning on windows?

Crola1702 commented 2 months ago

I got a question why CI https://github.com/ros2/rclcpp/pull/2443#issuecomment-1978752604 on https://github.com/ros2/rclcpp/pull/2443 could not detect this warning on windows?

This is because CI builds (such as the ones you linked) use sequential executor. Nightly builds are using parallel executor.

fujitatomoya commented 2 months ago

@thomasmoore-torc i think https://github.com/ros2/rclcpp/pull/2498 should fix our CI failure, let me try run CI again.

fujitatomoya commented 2 months ago

CI:

thomasmoore-torc commented 2 months ago

@fujitatomoya - it looks like that didn't fix it. While it shouldn't be an issue, the implementation of struct TypeAdapter<std::string, rclcpp::msg::LargeMessage> would generally be considered dangerous because it performs blind memcpy operations. It might be worth trying the following change to this code:

  template<>
  struct TypeAdapter<std::string, rclcpp::msg::LargeMessage>
  {
    using is_specialized = std::true_type;
    using custom_type = std::string;
    using ros_message_type = rclcpp::msg::LargeMessage;

    static void
    convert_to_ros_message(
      const custom_type & source,
      ros_message_type & destination)
    {
-     destination.size = source.size();
-     std::memcpy(destination.data.data(), source.data(), source.size());
+     destination.size = std::min(source.size(), destination.data.max_size());
+     std::memcpy(destination.data.data(), source.data(), destination.size);
    }

    static void
    convert_to_custom(
      const ros_message_type & source,
      custom_type & destination)
    {
      destination.resize(source.size);
      std::memcpy(destination.data(), source.data.data(), source.size);
    }
  };
fujitatomoya commented 2 months ago

CI:

clalancette commented 2 months ago

I poked at this a bit on my Windows VM, and it really doesn't like the change to create_ros_message_unique_ptr for some reason.

In particular, when I built this in Windows Debug, I got the following stack trace:

minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp(904) : Assertion failed: _CrtIsValidHeapPointer(block)
minkernel\crts\ucrt\src\appcrt\heap\debug_heap.cpp(908) : Assertion failed: is_block_type_valid(header->_block_use)
unknown file: error: SEH exception with code 0xc0000005 thrown in the test body.

While I don't know exactly what it means, it suggests to me that create_ros_message_unique_ptr isn't actually allocating the memory we think it is. I think someone needs to go in and validate what this line really allocates.