Remove memory leaks by ahcorde · Pull Request #186 · ros/class_loader

ahcorde · 2021-06-24T11:06:52Z

Signed-off-by: ahcorde ahcorde@gmail.com

I ran class_loader with asan and I found some memory leaks.

There is still one memory leak when we create unmanaged instance, the way to reproduce this leak is to run basicLoadUnmanaged and noWarningOnLazyLoad tests at the same time.

what is the right way to destroy this memory ?

hidmic · 2021-06-24T15:51:12Z

include/class_loader/class_loader.hpp

+          unloadLibraryInternal(false);
+        }
+        if (remove_when_possible) {
+          delete this;


@ahcorde this is very unusual. I'd suggest we use an std::shared_ptr to track ClassLoader ownership and defer cleanup tasks to its destructor.

hidmic · 2021-06-24T15:54:34Z

include/class_loader/class_loader.hpp

  template<class Base>
  Base * createUnmanagedInstance(const std::string & derived_class_name)
  {
+    class_loader_ref_count--;


@ahcorde to compensate for the ref count increase in createRawInstance? What if a plugin is simultaneously deleted? I'd strongly suggest not creating these glitches in the ref count.

hidmic · 2021-06-24T16:02:20Z

src/class_loader_core.cpp

            ", keeping library %s open.",
            library_path.c_str());
        }
+        purgeGraveyardOfMetaobjects(library_path, loader, true);


@ahcorde hmm, I think this was not here on purpose:

class_loader/src/class_loader_core.cpp

Lines 235 to 242 in 7b03fe3

// Insert into graveyard

// We remove the metaobject from its factory map, but we don't destroy it...instead it

// saved to a "graveyard" to the side.

// This is due to our static global variable initialization problem that causes factories

// to not be registered when a library is closed and then reopened.

// This is because it's truly not closed due to the use of global symbol binding i.e.

// calling dlopen with RTLD_GLOBAL instead of RTLD_LOCAL.

// We require using the former as the which is required to support RTTI

But I wonder why

class_loader/test/unique_ptr_test.cpp

Line 174 in 471c398

TEST(ClassLoaderUniquePtrTest, loadRefCountingLazy) {

isn't failing for you though.

What about this @ahcorde ?

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-06-25T08:30:20Z

--packages-above-and-dependencies class_loader

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: ahcorde <ahcorde@gmail.com>

include/class_loader/class_loader.hpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

include/class_loader/class_loader.hpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

include/class_loader/class_loader.hpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

hidmic

Once comments are addressed and CI goes green, I'm onboard with the change.

Signed-off-by: ahcorde <ahcorde@gmail.com>

hidmic

uncrustify is angry again

include/class_loader/class_loader.hpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-09-21T11:20:21Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-10-26T18:28:39Z

Rerunning tests. Anyhow failures look unrelared.

Linux
Linux-aarch64
macOS
Windows

hidmic

@ahcorde LGTM but for a question. Also, we should probably open a ticket to come back after Humble and remove the raw pointer interface.

hidmic · 2021-11-01T17:49:48Z

include/class_loader/class_loader.hpp

+ * This class inherit from enable_shared_from_this which means we must used smart pointers,
+ * otherwise the code might work but it may run into a leak.


@ahcorde nit:

Suggested change

* This class inherit from enable_shared_from_this which means we must used smart pointers,

* otherwise the code might work but it may run into a leak.

* This class inherits from `std::enable_shared_from_this`. It should be instantiated using an `std::shared_ptr`.

* Code will otherwise work but memory may be leaked.

hidmic · 2021-11-01T17:54:37Z

src/class_loader_core.cpp

            ", keeping library %s open.",
            library_path.c_str());
        }
+        purgeGraveyardOfMetaobjects(library_path, loader, true);


What about this @ahcorde ?

tylerjw · 2022-06-13T01:49:15Z

src/meta_object.cpp

+  for (unsigned int i = 0; i < impl_->associated_class_loaders_.size(); i++) {
+    delete impl_->associated_class_loaders_[i];
+  }


I don't understand how this doesn't cause problems unless these AbstractMetaObjectBase are never destroyed while the program is running?

Are these not the same ClassLoaders you are storing in shared_ptr objects?

ahcorde requested a review from hidmic June 24, 2021 11:06

ahcorde self-assigned this Jun 24, 2021

hidmic reviewed Jun 24, 2021

View reviewed changes

Improve memory management

0969655

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/leaks branch from 22de06e to 0969655 Compare June 25, 2021 08:28

some fixes

58d57ed

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from hidmic June 25, 2021 09:12

hidmic reviewed Jun 25, 2021

View reviewed changes

include/class_loader/class_loader.hpp Outdated Show resolved Hide resolved

include/class_loader/class_loader.hpp Outdated Show resolved Hide resolved

Use exceptions

6a800a4

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from hidmic June 25, 2021 14:11

hidmic reviewed Jun 28, 2021

View reviewed changes

include/class_loader/class_loader.hpp Outdated Show resolved Hide resolved

include/class_loader/class_loader.hpp Show resolved Hide resolved

Added comments and docs

97a4313

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from hidmic September 17, 2021 14:38

hidmic reviewed Sep 17, 2021

View reviewed changes

include/class_loader/class_loader.hpp Outdated Show resolved Hide resolved

make linters happy

6ced46e

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/leaks branch from 563ef2b to 6ced46e Compare September 20, 2021 08:24

ahcorde requested a review from hidmic September 20, 2021 08:25

hidmic requested review from clalancette and wjwwood September 20, 2021 12:18

hidmic reviewed Sep 20, 2021

View reviewed changes

added feedback

a17bcc3

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from hidmic September 20, 2021 16:09

hidmic reviewed Sep 20, 2021

View reviewed changes

include/class_loader/class_loader.hpp Outdated Show resolved Hide resolved

style

457d049

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from hidmic September 21, 2021 08:51

fixed warning

7c78923

Signed-off-by: ahcorde <ahcorde@gmail.com>

hidmic reviewed Nov 1, 2021

View reviewed changes

bpwilcox mentioned this pull request May 12, 2022

Remove raw pointer interface #197

Open

tylerjw mentioned this pull request Jun 12, 2022

Thread and Address Sanitizer CI #198

Merged

tylerjw reviewed Jun 13, 2022

View reviewed changes

audrow changed the base branch from ros2 to rolling June 28, 2022 14:17

	// Insert into graveyard
	// We remove the metaobject from its factory map, but we don't destroy it...instead it
	// saved to a "graveyard" to the side.
	// This is due to our static global variable initialization problem that causes factories
	// to not be registered when a library is closed and then reopened.
	// This is because it's truly not closed due to the use of global symbol binding i.e.
	// calling dlopen with RTLD_GLOBAL instead of RTLD_LOCAL.
	// We require using the former as the which is required to support RTTI

		* This class inherit from enable_shared_from_this which means we must used smart pointers,
		* otherwise the code might work but it may run into a leak.

Conversation

ahcorde commented Jun 24, 2021

Uh oh!

hidmic Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

ahcorde commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahcorde commented Sep 21, 2021

Uh oh!

ahcorde commented Oct 26, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

tylerjw Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ahcorde commented Jun 25, 2021 •

edited

Loading