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

[AIST-QA] Fix memory leak issue in rdf_loader #1828

Conversation

max-krichenbauer
Copy link
Contributor

Description

In the constructor of RDFLoader class, when umodel can not initialized, an error is shown and the constructor returns after showing an error message. In this case however, the umodel variable is not deleted which causes a memory leak.
This fix adds delete umodel; to the scope when constructor terminate prematurely.

This contribution is made by AIST ( https://www.aist.go.jp ) based on static code analysis with klocwork (Perforce Software).

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

- Correct the indication by the static analysis tool.
@rhaschke
Copy link
Contributor

rhaschke commented Jan 9, 2020

Thanks for pointing out this issue. I proposed a cleaner solution for this, avoiding new/delete completely in #1830. Closing here.

@rhaschke rhaschke closed this Jan 9, 2020
@max-krichenbauer
Copy link
Contributor Author

Thank you for the review and for proposing a better way to fix it!
Generally though, it would be much appreciated if you would allow us to modify this PR to meet your expectations instead of closing it and opening a new PR, because these PRs are used internally by our project management to evaluate if our efforts are effective and valued by the community.

@rhaschke
Copy link
Contributor

rhaschke commented Jan 9, 2020

My policy for such minor fixups is to apply them directly (particularly if I opened the source editor anyway to see the context). Usually I push those changes to the PR branch to keep them with the original PR. However, your PR was filed on an organizational account and github forbids pushing to those.
If you prefer adapting the PR yourself, I can try to remember this in future.

@max-krichenbauer
Copy link
Contributor Author

Thank you! I understand that it's probably annoying for you to go back-and-forth with PRs instead of just fixing things immediately, but for us it's hard to justify our work if our PRs are apparently being rejected.
(Of course, if the content of the contribution is not considered useful, feel free to reject PRs).

We're trying our best to be as responsive as possible, so if it's not too much of a hassle, could we be allowed to modify this PR?

@rhaschke
Copy link
Contributor

rhaschke commented Jan 9, 2020

Feel free to just fetch my additional changes in #1830 on top of your PR here.

@rhaschke rhaschke reopened this Jan 9, 2020
@max-krichenbauer
Copy link
Contributor Author

Thanks a lot!
Just one minute...

@rhaschke
Copy link
Contributor

rhaschke commented Jan 9, 2020

Sorry, I used the wrong type names. I hope, #1830 is fixed now (wait for Travis to finish).

@max-krichenbauer
Copy link
Contributor Author

Thanks! The checks on #1830 passed, so I applied the changes to this PR.

Thank you again so much for your understanding!

@rhaschke
Copy link
Contributor

rhaschke commented Jan 9, 2020

Thanks. Please remove the unrelated cleanup in end_effectors_widget.cpp.

@max-krichenbauer
Copy link
Contributor Author

Got it. Sorry for the misunderstanding.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Fine with me if Travis is happy as well.

@rhaschke rhaschke merged commit 4cb4485 into moveit:melodic-devel Jan 9, 2020
@max-krichenbauer max-krichenbauer deleted the feature/analysis_tool/melodic/error/Memory_Leak/moveit_ros branch January 9, 2020 11:58
rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Jan 9, 2020
Use smart pointers to avoid explicit new/delete
(and fix missing deletion of urdf model in error case).
Explicitly express with the code that the internal pointers urdf_ and srdf_ should
be initialized only if their initialization was successful.
rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request Jan 9, 2020
Use smart pointers to avoid explicit new/delete
(and fix missing deletion of urdf model in error case).
Explicitly express with the code that the internal pointers urdf_ and srdf_ should
be initialized only if their initialization was successful.
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
This ensures the color reset is applied because printing the color
reset after new lines seems to preven the color from actually being reset.

Co-authored-by: William Wedler <william.wedler@resquared.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.

None yet

3 participants