fix memory leaks / ClassLoader SEVERE warnings #1104

rhaschke · 2018-10-20T11:38:51Z

When move_group node finishes, there are always warnings like:

[ WARN] ros.rosconsole_bridge.class_loader.ClassLoader: SEVERE WARNING!!!
Attempting to unload library while objects created by this loader exist in the heap!
You should delete your objects before attempting to unload the library or destroying the ClassLoader.
The library will NOT be unloaded.

This is a hint that managed plugin instances are not correctly released before the corresponding plugin loader is. Even worse, MoveIt still used a lot of pointers created unmanaged, where we wouldn't even notice such issues. For this reason, use of pluginlib's createUnmanagedInstance() is strongly discouraged:
http://wiki.ros.org/class_loader#Understanding_Loading_and_Unloading

This PR is to be considered as an open branch to contribute to fixing these issues. I started by replacing
createUnmanagedInstance() with createUniqueInstance(), which is available for C++11.

I tracked down one issue already: The PlanningScenePtr passed to the ompl_simple_setup_ via ob::GoalPtr in ModelBasedPlanningContext::setGoalConstraints() was never released. This is an issue in OMPL.

As a quick fix, I changed the ConstraintSampler to hold a raw PlanningScene* instead of a shared_ptr.
This solved the memory leak: the PlanningScene instance is correctly released.
However, it's not a clean solution: There is no chance to validate the pointer before use. I suggest to use a weak_ptr instead, making scene_ private and only accessible via getPlanningScene(), which in turn could validate the weak pointer and issue an error msg (before eventually crashing).

Please keep commit history when merging.

rhaschke · 2018-10-20T11:41:30Z

Next issue is that the RobotModelPtr that was passed to the OMPL plugin, is not fully released.

v4hn · 2018-10-23T15:16:42Z

Do you propose to leave this pull-request open?

I would say it makes more sense to merge all the problems we find but have one issue to refer to...

rhaschke · 2018-10-23T15:35:11Z

Yes, I intended this PR as open to all contributors willing to tackle some of the mentioned issues ;-)

rhaschke · 2018-10-25T14:29:34Z

Related to #150 (comment), #150 (comment).

rhaschke · 2018-10-25T20:55:58Z

Fixed two other bugs:

KinematicsPluginLoader kept all it's cache instances
Planner plugins might not correctly release the passed RobotModel

simonschmeisser · 2018-10-26T13:17:10Z

moveit_ros/planning/kinematics_plugin_loader/src/kinematics_plugin_loader.cpp

@@ -239,8 +233,9 @@ class KinematicsPluginLoader::KinematicsLoaderImpl
  std::map<std::string, std::vector<std::string> > iksolver_to_tip_links_;  // a map between each ik solver and a vector
                                                                            // of custom-specified tip link(s)
  std::shared_ptr<pluginlib::ClassLoader<kinematics::KinematicsBase> > kinematics_loader_;
-  std::map<const robot_model::JointModelGroup*, std::vector<kinematics::KinematicsBasePtr> > instances_;
+  std::map<const robot_model::JointModelGroup*, kinematics::KinematicsBasePtr> instances_;


I had the impression that multiple plugins can be loaded for the same JointModelGroup* (for whatever reason). This seems to change that. Is that intentional?

Sorry, I only explained the reasoning for this in #150 (comment)

The KinematicsPluginLoader caches created kinematics solver (KS) instances. But the cache only maintains unique instances - they are not shared. The only purpose of the cache is to not allocate a KS twice in a row - once for validating that it is suitable for JMG here and once for actual use here.
Eventually, the JMG owns the created KS instance. KPL only caches it for short term.
As I got the very same impression as @simonschmeisser in the first place, I decided to emphasize the fact that this is only short-term cache by changing the data structure - only the last created KS instance is held per JMG (and passed on to the caller in the 2nd call in a row).
This also resolves a potential issue with dangling shared_ptrs in the cache (which usually should only be weak_ptrs for this reason).

Just as a side note: Indeed each JMG owns its unique solver instance. This helps to avoid race conditions when using them from multiple threads in parallel.

rhaschke · 2018-11-12T00:33:42Z

Tracked down next bug: MoveGroupInterface doesn't release RobotModel passed from rviz motion planning plugin.

rhaschke · 2018-11-12T16:08:30Z

@v4hn From my point of view, this is now ready to merge. Together with #1166 this fixes all class-loader warnings.

rhaschke · 2018-11-19T16:34:37Z

Rebased to resolve conflicts.

rhaschke · 2018-11-26T08:32:18Z

Ping @v4hn. I would like to merge this into the next release.

v4hn · 2018-11-26T17:53:33Z

gimme a break, I spent all day looking through requests.

I will try to address more open requests tomorrow.

rhaschke · 2018-12-04T10:08:39Z

@v4hn Ping.

use of pluginlib's createUnmanagedInstance() is strongly discouraged: http://wiki.ros.org/class_loader#Understanding_Loading_and_Unloading

rhaschke · 2018-12-07T15:33:17Z

@v4hn I just found another improvement. Working on it later. Please do not yet merge immediately.

SharedStorage acts as a cache for several shared_ptrs. However, a cache should always store its pointers as weak_ptrs!

ompl_simple_setup_ is stored in ModelBasedPlanningContextSpecification. Additionally a functor was passed to ompl_simple_setup_, storing a copy of the spec (and thus the shared_ptr). Resolved, by storing a const reference only.

rhaschke · 2018-12-08T02:18:52Z

Turned out, that the main bug for classloader issues was a circular reference in the OMPL wrapper: ed3f4b4

…g chomp modules)

v4hn · 2018-12-10T14:43:41Z

Sorry for not looking through this in time. Post-merge the changes seem valid and I would have merged.

At the same time I really don't think it's a good idea to merge architecture changes in memory management (changing shared for weak ptrs, etc) an hour before a release.

rhaschke · 2018-12-10T15:48:00Z

I don't think it's a good idea to merge architecture changes in memory management just before a release.

These changes are more than a month old and I'm running this code in production since then. I wasn't afraid at all ;-)

v4hn · 2018-12-10T16:14:14Z

These changes are more than a month old and I'm running this code in production since then.

Yes, you run it in your environment.
The reason for our independent-second-approval-scheme is that environments&setups vary a lot between users (and maintainers). Shadow-fixed reaches more users to test changes, but a first independent test/review still makes sense to avoid hotfix commits/releases.

In this case I agree post-merge, so we would have merged (& released) it either way..

rhaschke · 2018-12-10T16:34:51Z

Yes, you run it in your environment.

Touché.

rhaschke mentioned this pull request Oct 20, 2018

memory leak in ompl_simple_setup_ ubi-agni/moveit_task_constructor#5

Closed

rhaschke force-pushed the fix-memory-leaks branch from 9d6d0e6 to 8ec0d0a Compare October 20, 2018 15:21

rhaschke mentioned this pull request Oct 25, 2018

Pass RobotModel to kinematics plugins #150

Closed

simonschmeisser reviewed Oct 26, 2018

View reviewed changes

rhaschke force-pushed the fix-memory-leaks branch from b7c3461 to 60ff421 Compare October 26, 2018 13:34

rhaschke force-pushed the fix-memory-leaks branch from 60ff421 to 66d07e2 Compare November 11, 2018 15:32

rhaschke changed the title ~~WIP: fix memory leaks / ClassLoader SEVERE warnings~~ fix memory leaks / ClassLoader SEVERE warnings Nov 11, 2018

rhaschke force-pushed the fix-memory-leaks branch from 66d07e2 to 4e9f65a Compare November 11, 2018 16:28

rhaschke force-pushed the fix-memory-leaks branch 3 times, most recently from 540c553 to bfb0964 Compare November 12, 2018 13:45

rhaschke force-pushed the fix-memory-leaks branch from bfb0964 to 6167e7c Compare November 19, 2018 16:34

rhaschke force-pushed the fix-memory-leaks branch from 6167e7c to 44b03ed Compare November 22, 2018 09:31

rhaschke mentioned this pull request Nov 26, 2018

201811 release #1225

Closed

12 tasks

This was referenced Nov 28, 2018

Replace createUnmanagedInstance with createUniqueInstance #895

Closed

Melodic: Unmanaged instances to std::shared_ptr? #866

Closed

plugins: use createUniqueInstance()

c4d44e2

use of pluginlib's createUnmanagedInstance() is strongly discouraged: http://wiki.ros.org/class_loader#Understanding_Loading_and_Unloading

KinematicsPluginLoader: only cache the latest instance

7018061

rhaschke force-pushed the fix-memory-leaks branch from 44b03ed to 59f2625 Compare December 7, 2018 21:51

fix SharedStorage: use weak_ptrs

d0daf64

SharedStorage acts as a cache for several shared_ptrs. However, a cache should always store its pointers as weak_ptrs!

rhaschke force-pushed the fix-memory-leaks branch from 59f2625 to 9e1e088 Compare December 8, 2018 01:49

rhaschke force-pushed the fix-memory-leaks branch from 9e1e088 to ed3f4b4 Compare December 8, 2018 02:07

rhaschke merged commit ed3f4b4 into moveit:melodic-devel Dec 9, 2018

rhaschke added a commit that referenced this pull request Dec 9, 2018

Merge pull requests #1104 (fixing memory leaks) and #1251 (rearrangin…

8d4c59f

…g chomp modules)

rhaschke deleted the fix-memory-leaks branch December 9, 2018 23:28

rhaschke mentioned this pull request Dec 11, 2018

Flaky unittests #1259

Closed

v4hn mentioned this pull request Dec 12, 2018

Fix flaky test #1261

Merged

rhaschke mentioned this pull request Jul 2, 2019

Allow ROS namespaces for planning request adapters: pass a NodeHandle for their initialization #1530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix memory leaks / ClassLoader SEVERE warnings #1104

fix memory leaks / ClassLoader SEVERE warnings #1104

rhaschke commented Oct 20, 2018 •

edited

Loading

rhaschke commented Oct 20, 2018 •

edited

Loading

v4hn commented Oct 23, 2018

rhaschke commented Oct 23, 2018

rhaschke commented Oct 25, 2018

rhaschke commented Oct 25, 2018

simonschmeisser Oct 26, 2018

rhaschke Oct 26, 2018

rhaschke Oct 26, 2018

rhaschke commented Nov 12, 2018

rhaschke commented Nov 12, 2018

rhaschke commented Nov 19, 2018

rhaschke commented Nov 26, 2018

v4hn commented Nov 26, 2018

rhaschke commented Dec 4, 2018

rhaschke commented Dec 7, 2018

rhaschke commented Dec 8, 2018

v4hn commented Dec 10, 2018

rhaschke commented Dec 10, 2018

v4hn commented Dec 10, 2018

rhaschke commented Dec 10, 2018

fix memory leaks / ClassLoader SEVERE warnings #1104

fix memory leaks / ClassLoader SEVERE warnings #1104

Conversation

rhaschke commented Oct 20, 2018 • edited Loading

rhaschke commented Oct 20, 2018 • edited Loading

v4hn commented Oct 23, 2018

rhaschke commented Oct 23, 2018

rhaschke commented Oct 25, 2018

rhaschke commented Oct 25, 2018

simonschmeisser Oct 26, 2018

Choose a reason for hiding this comment

rhaschke Oct 26, 2018

Choose a reason for hiding this comment

rhaschke Oct 26, 2018

Choose a reason for hiding this comment

rhaschke commented Nov 12, 2018

rhaschke commented Nov 12, 2018

rhaschke commented Nov 19, 2018

rhaschke commented Nov 26, 2018

v4hn commented Nov 26, 2018

rhaschke commented Dec 4, 2018

rhaschke commented Dec 7, 2018

rhaschke commented Dec 8, 2018

v4hn commented Dec 10, 2018

rhaschke commented Dec 10, 2018

v4hn commented Dec 10, 2018

rhaschke commented Dec 10, 2018

rhaschke commented Oct 20, 2018 •

edited

Loading

rhaschke commented Oct 20, 2018 •

edited

Loading