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.

Sign up for GitHub

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

Remove memory leaks #186

Open
wants to merge 8 commits into
base: rolling
Choose a base branch
from
Open

Remove memory leaks #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0969655
Improve memory management
ahcorde Jun 25, 2021
58d57ed
some fixes
ahcorde Jun 25, 2021
6a800a4
Use exceptions
ahcorde Jun 25, 2021
97a4313
Added comments and docs
ahcorde Sep 17, 2021
6ced46e
make linters happy
ahcorde Sep 20, 2021
a17bcc3
added feedback
ahcorde Sep 20, 2021
457d049
style
ahcorde Sep 21, 2021
7c78923
fixed warning
ahcorde Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 41 additions & 9 deletions include/class_loader/class_loader.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,10 @@ std::string systemLibraryFormat(const std::string & library_name);
* @brief This class allows loading and unloading of dynamically linked libraries which contain class
* definitions from which objects can be created/destroyed during runtime (i.e. class_loader).
* Libraries loaded by a ClassLoader are only accessible within scope of that ClassLoader object.
* 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.
Comment on lines +78 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

*/
class ClassLoader
class ClassLoader : public std::enable_shared_from_this<ClassLoader>
{
public:
template<typename Base>
Expand Down Expand Up @@ -124,10 +126,25 @@ class ClassLoader
template<class Base>
std::shared_ptr<Base> createInstance(const std::string & derived_class_name)
{
return std::shared_ptr<Base>(
createRawInstance<Base>(derived_class_name, true),
std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
);
try {
return std::shared_ptr<Base>(
createRawInstance<Base>(derived_class_name, true),
std::bind(&ClassLoader::onPluginDeletion<Base>, shared_from_this(), std::placeholders::_1)
);
} catch (std::bad_weak_ptr &) { // This is not a shared_ptr
static bool create_instance_message = false;
if (!create_instance_message) {
CONSOLE_BRIDGE_logWarn(
"To createInstance() with a class_loader::ClassLoader "
"instance whose lifetime is not managed by an std::shared_ptr "
"is deprecated.");
create_instance_message = true;
}
return std::shared_ptr<Base>(
createRawInstance<Base>(derived_class_name, true),
std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
);
}
}

/// Generates an instance of loadable classes (i.e. class_loader).
Expand All @@ -147,10 +164,25 @@ class ClassLoader
UniquePtr<Base> createUniqueInstance(const std::string & derived_class_name)
{
Base * raw = createRawInstance<Base>(derived_class_name, true);
return std::unique_ptr<Base, DeleterType<Base>>(
raw,
std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
);
try {
return std::unique_ptr<Base, DeleterType<Base>>(
raw,
std::bind(&ClassLoader::onPluginDeletion<Base>, shared_from_this(), std::placeholders::_1)
);
} catch (std::bad_weak_ptr &) { // This is not a shared_ptr
static bool create_unique_instance_message = false;
if (!create_unique_instance_message) {
CONSOLE_BRIDGE_logWarn(
"To createUniqueInstance() with a class_loader::ClassLoader "
"instance whose lifetime is not managed by an std::shared_ptr "
"is deprecated.");
create_unique_instance_message = true;
}
return std::unique_ptr<Base, DeleterType<Base>>(
raw,
std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
);
}
}

/// Generates an instance of loadable classes (i.e. class_loader).
Expand Down
23 changes: 12 additions & 11 deletions include/class_loader/multi_library_class_loader.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ namespace class_loader
{

typedef std::string LibraryPath;
typedef std::map<LibraryPath, class_loader::ClassLoader *> LibraryToClassLoaderMap;
typedef std::vector<ClassLoader *> ClassLoaderVector;
typedef std::map<LibraryPath, std::shared_ptr<class_loader::ClassLoader>> LibraryToClassLoaderMap;
typedef std::vector<std::shared_ptr<ClassLoader>> ClassLoaderVector;

class MultiLibraryClassLoaderImpl;

Expand Down Expand Up @@ -96,7 +96,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
"class_loader::MultiLibraryClassLoader: "
"Attempting to create instance of class type %s.",
class_name.c_str());
ClassLoader * loader = getClassLoaderForClass<Base>(class_name);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForClass<Base>(class_name);
if (nullptr == loader) {
throw class_loader::CreateClassException(
"MultiLibraryClassLoader: Could not create object of class type " +
Expand All @@ -121,7 +121,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
std::shared_ptr<Base> createInstance(
const std::string & class_name, const std::string & library_path)
{
ClassLoader * loader = getClassLoaderForLibrary(library_path);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForLibrary(library_path);
if (nullptr == loader) {
throw class_loader::NoClassLoaderExistsException(
"Could not create instance as there is no ClassLoader in "
Expand All @@ -146,7 +146,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
CONSOLE_BRIDGE_logDebug(
"class_loader::MultiLibraryClassLoader: Attempting to create instance of class type %s.",
class_name.c_str());
ClassLoader * loader = getClassLoaderForClass<Base>(class_name);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForClass<Base>(class_name);
if (nullptr == loader) {
throw class_loader::CreateClassException(
"MultiLibraryClassLoader: Could not create object of class type " + class_name +
Expand All @@ -170,7 +170,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
ClassLoader::UniquePtr<Base>
createUniqueInstance(const std::string & class_name, const std::string & library_path)
{
ClassLoader * loader = getClassLoaderForLibrary(library_path);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForLibrary(library_path);
if (nullptr == loader) {
throw class_loader::NoClassLoaderExistsException(
"Could not create instance as there is no ClassLoader in "
Expand All @@ -193,7 +193,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
template<class Base>
Base * createUnmanagedInstance(const std::string & class_name)
{
ClassLoader * loader = getClassLoaderForClass<Base>(class_name);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForClass<Base>(class_name);
if (nullptr == loader) {
throw class_loader::CreateClassException(
"MultiLibraryClassLoader: Could not create class of type " + class_name);
Expand All @@ -213,7 +213,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
template<class Base>
Base * createUnmanagedInstance(const std::string & class_name, const std::string & library_path)
{
ClassLoader * loader = getClassLoaderForLibrary(library_path);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForLibrary(library_path);
if (nullptr == loader) {
throw class_loader::NoClassLoaderExistsException(
"Could not create instance as there is no ClassLoader in MultiLibraryClassLoader "
Expand Down Expand Up @@ -273,7 +273,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
template<class Base>
std::vector<std::string> getAvailableClassesForLibrary(const std::string & library_path)
{
ClassLoader * loader = getClassLoaderForLibrary(library_path);
std::shared_ptr<class_loader::ClassLoader> loader = getClassLoaderForLibrary(library_path);
if (nullptr == loader) {
throw class_loader::NoClassLoaderExistsException(
"There is no ClassLoader in MultiLibraryClassLoader bound to library " +
Expand Down Expand Up @@ -321,15 +321,16 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
* @param library_path - the library from which we want to create the plugin
* @return A pointer to the ClassLoader*, == nullptr if not found
*/
ClassLoader * getClassLoaderForLibrary(const std::string & library_path);
std::shared_ptr<class_loader::ClassLoader> getClassLoaderForLibrary(
const std::string & library_path);

/// Gets a handle to the class loader corresponding to a specific class.
/**
* @param class_name name of class for which we want to create instance.
* @return A pointer to the ClassLoader, or NULL if not found.
*/
template<typename Base>
ClassLoader * getClassLoaderForClass(const std::string & class_name)
std::shared_ptr<class_loader::ClassLoader> getClassLoaderForClass(const std::string & class_name)
{
ClassLoaderVector loaders = getAllAvailableClassLoaders();
for (ClassLoaderVector::iterator i = loaders.begin(); i != loaders.end(); ++i) {
Expand Down
1 change: 1 addition & 0 deletions src/class_loader_core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ void unloadLibrary(const std::string & library_path, ClassLoader * loader)
", keeping library %s open.",
library_path.c_str());
}
purgeGraveyardOfMetaobjects(library_path, loader, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about this @ahcorde ?

return;
} catch (const std::runtime_error & e) {
throw class_loader::LibraryUnloadException(
Expand Down
3 changes: 3 additions & 0 deletions src/meta_object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ AbstractMetaObjectBase::~AbstractMetaObjectBase()
"class_loader.impl.AbstractMetaObjectBase: "
"Destroying MetaObject %p (base = %s, derived = %s, library path = %s)",
this, baseClassName().c_str(), className().c_str(), getAssociatedLibraryPath().c_str());
for (unsigned int i = 0; i < impl_->associated_class_loaders_.size(); i++) {
delete impl_->associated_class_loaders_[i];
}
Comment on lines +74 to +76
Copy link

Choose a reason for hiding this comment

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

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?

delete impl_;
}

Expand Down
9 changes: 5 additions & 4 deletions src/multi_library_class_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "class_loader/multi_library_class_loader.hpp"

#include <cstddef>
#include <memory>
#include <mutex>
#include <string>
#include <vector>
Expand Down Expand Up @@ -68,7 +69,8 @@ std::vector<std::string> MultiLibraryClassLoader::getRegisteredLibraries() const
return libraries;
}

ClassLoader * MultiLibraryClassLoader::getClassLoaderForLibrary(const std::string & library_path)
std::shared_ptr<class_loader::ClassLoader> MultiLibraryClassLoader::getClassLoaderForLibrary(
const std::string & library_path)
{
return impl_->active_class_loaders_[library_path];
}
Expand All @@ -93,7 +95,7 @@ void MultiLibraryClassLoader::loadLibrary(const std::string & library_path)
{
if (!isLibraryAvailable(library_path)) {
impl_->active_class_loaders_[library_path] =
new class_loader::ClassLoader(library_path, isOnDemandLoadUnloadEnabled());
std::make_shared<class_loader::ClassLoader>(library_path, isOnDemandLoadUnloadEnabled());
}
}

Expand All @@ -108,11 +110,10 @@ int MultiLibraryClassLoader::unloadLibrary(const std::string & library_path)
{
int remaining_unloads = 0;
if (isLibraryAvailable(library_path)) {
ClassLoader * loader = getClassLoaderForLibrary(library_path);
auto loader = getClassLoaderForLibrary(library_path);
remaining_unloads = loader->unloadLibrary();
if (remaining_unloads == 0) {
impl_->active_class_loaders_[library_path] = nullptr;
delete (loader);
}
}
return remaining_unloads;
Expand Down