Skip to content

Fix a bug on invalid pointer address when using "MESSAGE" compressio…#866

Merged
emersonknapp merged 5 commits intoros2:masterfrom
Barry-Xu-2018:topic-fix-writer-interface-issue
Oct 18, 2021
Merged

Fix a bug on invalid pointer address when using "MESSAGE" compressio…#866
emersonknapp merged 5 commits intoros2:masterfrom
Barry-Xu-2018:topic-fix-writer-interface-issue

Conversation

@Barry-Xu-2018
Copy link
Copy Markdown
Contributor

Address #856

@Barry-Xu-2018 Barry-Xu-2018 requested a review from a team as a code owner September 6, 2021 05:26
@Barry-Xu-2018 Barry-Xu-2018 requested review from lihui815 and zmichaels11 and removed request for a team September 6, 2021 05:26
@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

Barry-Xu-2018 commented Sep 6, 2021

This modification affects the performance for a case No message compression and cache size is set to 0.
For this case, the message need not be duplicated.
If not change class Writer, I have no simple way to get the information of cache size in Writer::write.
So do duplication for all cases.

@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

I made new PR #867 to add new interface to help judgement whether message need to be duplicated.

Copy link
Copy Markdown
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This looks good - but is there any way to add a unit test for this case?

@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-fix-writer-interface-issue branch from 67f9778 to 45c9a78 Compare September 30, 2021 09:11
@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

@emersonknapp

I submit a new commit (45c9a78)
In this commit, I mark the old interface as deprecated and add a new write interface instead of that.
I think we should not make a lot of change for fixing the problem from deprecated interface.

@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

Barry-Xu-2018 commented Oct 4, 2021

After checking, the cause of failure is relevant to set -Werror=deprecated-declarations.
In code, I marked the old interface as deprecated.

… mode to write data

Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-fix-writer-interface-issue branch from 3563b2a to 161952b Compare October 9, 2021 10:23
@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

Do rebase

@emersonknapp
Copy link
Copy Markdown
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/61f3493495d5b27fb31cdd53f4d29334/raw/e057b1e91a54bdfd8b8b165e3d5fad7a61652ea0/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2 rosbag2_tests
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2 rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9174

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

" const std::string & topic_name," \
" const std::string & type_name," \
" const rclcpp::Time & time) instead."
)]]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Line 199 in this file is causing the Windows warning - since the "non-serialized ROS message" template function calls this "reference version".

I think that the solution is to create a shared_ptr to the SerializedBagMessage, instead of a local instance.

Something like:

  template<class MessageT>
  void write(
    const MessageT & message,
    const std::string & topic_name,
    const rclcpp::Time & time)
  {
    auto serialized_msg = std::make_shared<rclcpp::SerializedMessage>();
    rclcpp::Serialization<MessageT> serialization;
    serialization.serialize_message(&message, serialized_msg.get());
    return write(serialized_msg, topic_name, rosidl_generator_traits::name<MessageT>(), time);

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Copy Markdown
Contributor Author

After checking failure for Rpr__rosbag2__ubuntu_focal_amd64, failed cases are

projectroot.test_play_timing__rmw_cyclonedds_cpp
rosbag2_transport.PlayerTestFixture.playing_respects_delay

The failure cause isn't related to this PR.
In my local environment, I cannot reproduce this problem.

@emersonknapp
Copy link
Copy Markdown
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/01dc4976a14df75a802c9e347d0309f8/raw/e057b1e91a54bdfd8b8b165e3d5fad7a61652ea0/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2 rosbag2_tests
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2 rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9190

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants