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

resolve memory leak for serialized message #502

Merged
merged 2 commits into from
Aug 19, 2020
Merged

resolve memory leak for serialized message #502

merged 2 commits into from
Aug 19, 2020

Conversation

Karsten1987
Copy link
Collaborator

addressed #499

as stated in #499 (comment), the change to rclcpp::SerializedMessage introduced this regression in which we transfer ownership, the smart pointer however doesn't cleanup the transferred memory. Specifically this change here:
https://github.com/ros2/rosbag2/pull/386/files#diff-1a63843318260f13eda07d98c90c9c66L107-L116

@Karsten1987
Copy link
Collaborator Author

Karsten1987 commented Aug 10, 2020

build up to rosbag2, testing rosbag2_tests, rosbag2_transport

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

@Karsten1987 Karsten1987 mentioned this pull request Aug 10, 2020
@dawonn-haval
Copy link

I think you forgot to allocate 'msg'.

I got a segfault with this PR, so I copied a snippet from elsewhere and it seems to run without leaking:

      auto msg = new rcutils_uint8_array_t;
      *msg = rcutils_get_zero_initialized_uint8_array();

      bag_message->serialized_data = std::shared_ptr<rcutils_uint8_array_t>(
        msg,
        [](rcutils_uint8_array_t * msg) {
            int error = rcutils_uint8_array_fini(msg);
            delete msg;
            if (error != RCUTILS_RET_OK) {
              RCUTILS_LOG_ERROR_NAMED("rosbag2_transport", "Recorder::create_subscription leaking memory %i", error);
        }
        });
      *bag_message->serialized_data = message->release_rcl_serialized_message();

@Karsten1987
Copy link
Collaborator Author

Thanks for given it a shot. Can you post a backtrace or explain what you've ran to provoke the segfault?
It worked as-is on my machine with valgrind reporting improvements in terms of heap usage.

@dawonn-haval
Copy link

dawonn-haval commented Aug 11, 2020 via email

@Karsten1987
Copy link
Collaborator Author

my bad, this should be fixed now.

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 LGTM, but a comment explaining why it's necessary may be helpful

Karsten1987 and others added 2 commits August 19, 2020 10:40
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Collaborator Author

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

@Karsten1987 Karsten1987 merged commit 91bf0bf into master Aug 19, 2020
@Karsten1987 Karsten1987 deleted the fix_499 branch August 19, 2020 18:38
@clalancette
Copy link
Contributor

I suspect that this PR is the cause of a regression on the buildfarm last night: https://ci.ros2.org/view/nightly/job/nightly_linux_debug/1664/testReport/junit/(root)/projectroot/test_record_all_no_discovery__rmw_fastrtps_cpp/ . It's not immediately obvious to me why, but it is in the correct area and its the only rosbag2 change from yesterday.

Karsten1987 added a commit that referenced this pull request Aug 26, 2020
* resolve memory leak for serialized message

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* add comment

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Karsten1987 added a commit that referenced this pull request Aug 31, 2020
* resolve memory leak for serialized message

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* add comment

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
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.

4 participants