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

Corrected breaks when plotting array data #50

Merged
merged 3 commits into from
Jan 13, 2023
Merged

Corrected breaks when plotting array data #50

merged 3 commits into from
Jan 13, 2023

Conversation

dwbertol
Copy link
Contributor

@dwbertol dwbertol commented Dec 4, 2022

The plugin breaks due to memory leaking when the dds message has arrays. This is related to the utilization of DynamicData_ptr and asynchronous deletion of data.
This is in some way related to the same problem of eProsima/Fast-DDS#2970

Signed-off-by: Douglas Bertol [email protected]

This prevents memory leaking and makes the code faster.

Signed-off-by: Douglas Bertol <[email protected]>
@dwbertol dwbertol marked this pull request as ready for review December 4, 2022 09:24
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

Choose a reason for hiding this comment

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

Thank you @dwbertol for contributing to this repository, we greatly value every effort from the community to help us improve.

It turns out we have detected and fixed this issue in parallel at PR #51, but we would prefer to join yours once this review is addressed. If it is not possible for you to apply the requested suggestions, we would be willing to do so. But note that we have no permission as it is, hence another PR would need to be created from a branch other than main at your fork .

Thanks again, and wish you a happy new year!

@dwbertol
Copy link
Contributor Author

dwbertol commented Jan 4, 2023

Hello @juanlofer-eprosima. I have made the changes according to the review.

Happy new year!

Copy link
Contributor

@jparisu jparisu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

Choose a reason for hiding this comment

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

LGTM, good job!

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