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

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

Merged

Conversation

Barry-Xu-2018
Copy link
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
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
Contributor Author

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

Copy link
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
Copy link
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
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
Contributor Author

Do rebase

@emersonknapp
Copy link
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
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
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
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