search

moveit / moveit_ros

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
70 stars 118 forks source link

don't destroy SharedStorage #706

Closed rhaschke closed 8 years ago

rhaschke commented 8 years ago

… to avoid interference with destruction of class_loader's static variables This is actually a workaround for design issues in class_loader. Fixes #592. Replaces #702 and #705.

v4hn commented 8 years ago

That's the "hammer" fix and it resolves the classloader errors. :-) It's clearly a better solution than #705, although I'd like to get rid of destructors that only `*.reset()` anyway.. Even though I don't like the "stay with the ship until it crashes" type of programming at all, it's probably the best thing we can do until someone rewrote the class_loader code. Could you make the explanation a bit more specific though? I would really prefer something like

Upon destruction of a static SharedStorage, class_loader's static variables might already be destroyed and the destructor of the class_loader based kinematics plugin could fail.

Other than that, +1 indigo onward

rhaschke commented 8 years ago

Extended the comment. Have a look. I agree, that this is the hammer fix. But I don't see another option - despite fixing class_loader. #702 and #705 only work accidentally. I checked correctness of this PR with AddressSanitizer.

davetcoleman commented 8 years ago

lol I'm not sure the meaning of "hammer fix" ?

v4hn commented 8 years ago

Nice! I was about to merge it (after travis ran through), when I remembered Scott Meyer's A.: C++ and the perils of double-checked locking. GCC by default has thread-safe C++ static local initialization, so could you please rewrite it to

static SharedStorage *storage = new SharedStorage;

to make the whole thing somewhat thread-safe and remove the condition? ("somewhat" because c++98 does not enforce it while gcc implements it by default).

@davetcoleman: it means "why use a scalpel if a hammer does the job?", i.e. "why care for memory management when the OS takes care of it after the process finished?"

rhaschke commented 8 years ago

Interesting article. I changed the code appropriately.

v4hn commented 8 years ago

merged into jade and picked into indigo after testing. @alainsanguinetti this should finally resolve the bug you reported. Now we have a memory leak, but at least nothing crashes/complains anymore.

rhaschke commented 8 years ago

Please note, that the memory leak is only "formal". The SharedStorage would be maintained until the end of the process anyway. There is no continuous growth of memory usage!