Fix memory leaks and lifetime bugs

Signed-off-by: Tyler Weaver <[email protected]>

Author: tylerjw

CMakeLists.txt

@@ -4,7 +4,7 @@ project(class_loader)
 
 # Default to C++14
 if(NOT CMAKE_CXX_STANDARD)
-  set(CMAKE_CXX_STANDARD 14)
+  set(CMAKE_CXX_STANDARD 17)
 endif()
 if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   add_compile_options(-Wall -Wextra -Wpedantic)

include/class_loader/class_loader.hpp

@@ -76,7 +76,7 @@ std::string systemLibraryFormat(const std::string & library_name);
  * 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.
  */
-class ClassLoader
+class ClassLoader : public std::enable_shared_from_this<ClassLoader>
 {
 public:
   template<typename Base>
@@ -86,14 +86,17 @@ class ClassLoader
   using UniquePtr = std::unique_ptr<Base, DeleterType<Base>>;
 
   /**
-   * @brief Constructor for ClassLoader
+   * @brief This is required (as apposed to the old constructor) to enforce that the lifetime of the ClassLoader
+   * can be preserved by objects that this class creates.
    *
    * @param library_path - The path of the runtime library to load
    * @param ondemand_load_unload - Indicates if on-demand (lazy) unloading/loading of libraries
    * occurs as plugins are created/destroyed.
    */
   CLASS_LOADER_PUBLIC
-  explicit ClassLoader(const std::string & library_path, bool ondemand_load_unload = false);
+  [[nodiscard]] static std::shared_ptr<ClassLoader> Make(
+    const std::string & library_path,
+    bool ondemand_load_unload = false);
 
   /**
    * @brief Destructor for ClassLoader. All libraries opened by this ClassLoader are unloaded automatically.
@@ -109,7 +112,7 @@ class ClassLoader
   template<class Base>
   std::vector<std::string> getAvailableClasses() const
   {
-    return class_loader::impl::getAvailableClasses<Base>(this);
+    return class_loader::impl::getAvailableClasses<Base>(shared_from_this());
   }
 
   /**
@@ -126,7 +129,9 @@ class ClassLoader
   {
     return std::shared_ptr<Base>(
       createRawInstance<Base>(derived_class_name, true),
-      std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
+      [class_loader = shared_from_this()](Base * obj) {
+        return class_loader->onPluginDeletion<Base>(obj);
+      }
     );
   }
 
@@ -149,7 +154,9 @@ class ClassLoader
     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)
+      [class_loader = shared_from_this()](Base * obj) {
+        return class_loader->onPluginDeletion<Base>(obj);
+      }
     );
   }
 
@@ -253,6 +260,16 @@ class ClassLoader
   int unloadLibrary();
 
 private:
+  /**
+   * @brief Private Constructor for ClassLoader
+   *
+   * @param library_path - The path of the runtime library to load
+   * @param ondemand_load_unload - Indicates if on-demand (lazy) unloading/loading of libraries
+   * occurs as plugins are created/destroyed.
+   */
+  CLASS_LOADER_PUBLIC
+  explicit ClassLoader(const std::string & library_path, bool ondemand_load_unload = false);
+
   /**
    * @brief Callback method when a plugin created by this class loader is destroyed
    *
@@ -324,7 +341,7 @@ class ClassLoader
       loadLibrary();
     }
 
-    Base * obj = class_loader::impl::createInstance<Base>(derived_class_name, this);
+    Base * obj = class_loader::impl::createInstance<Base>(derived_class_name, shared_from_this());
     assert(obj != NULL);  // Unreachable assertion if createInstance() throws on failure.
 
     if (managed) {

include/class_loader/class_loader_core.hpp

@@ -76,11 +76,14 @@ namespace impl
 typedef std::string LibraryPath;
 typedef std::string ClassName;
 typedef std::string BaseClassName;
-typedef std::map<ClassName, impl::AbstractMetaObjectBase *> FactoryMap;
+typedef std::map<ClassName, std::shared_ptr<AbstractMetaObjectBase>> FactoryMap;
 typedef std::map<BaseClassName, FactoryMap> BaseToFactoryMapMap;
 typedef std::pair<LibraryPath, std::shared_ptr<rcpputils::SharedLibrary>> LibraryPair;
 typedef std::vector<LibraryPair> LibraryVector;
-typedef std::vector<AbstractMetaObjectBase *> MetaObjectVector;
+typedef std::vector<std::shared_ptr<AbstractMetaObjectBase>> MetaObjectVector;
+
+CLASS_LOADER_PUBLIC
+MetaObjectVector & getMetaObjectGraveyard();
 
 CLASS_LOADER_PUBLIC
 void printDebugInfoToScreen();
@@ -139,8 +142,15 @@ ClassLoader * getCurrentlyActiveClassLoader();
  * @param loader - pointer to the currently active ClassLoader.
  */
 CLASS_LOADER_PUBLIC
-void setCurrentlyActiveClassLoader(ClassLoader * loader);
+void setCurrentlyActiveClassLoader(std::shared_ptr<ClassLoader> loader);
 
+/**
+ * @brief Inserts meta object into the graveyard to preserve the lifetime.
+ *
+ * @param meta_obj - pointer to the meta object.
+ */
+CLASS_LOADER_PUBLIC
+void insertMetaObjectIntoGraveyard(std::shared_ptr<AbstractMetaObjectBase> meta_obj);
 
 /**
  * @brief This function extracts a reference to the FactoryMap for appropriate base class out of
@@ -240,8 +250,8 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla
   }
 
   // Create factory
-  impl::AbstractMetaObject<Base> * new_factory =
-    new impl::MetaObject<Derived, Base>(class_name, base_class_name);
+  auto new_factory =
+    std::make_shared<impl::MetaObject<Derived, Base>>(class_name, base_class_name);
   new_factory->addOwningClassLoader(getCurrentlyActiveClassLoader());
   new_factory->setAssociatedLibraryPath(getCurrentlyLoadingLibraryName());
 
@@ -260,13 +270,17 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla
       "and use either class_loader::ClassLoader/MultiLibraryClassLoader to open.",
       class_name.c_str());
   }
+
+  // We insert every factory into the graveyard to preserve the lifetime of all meta objects
+  // until the process exits.
+  insertMetaObjectIntoGraveyard(new_factory);
   factoryMap[class_name] = new_factory;
   getPluginBaseToFactoryMapMapMutex().unlock();
 
   CONSOLE_BRIDGE_logDebug(
     "class_loader.impl: "
     "Registration of %s complete (Metaobject Address = %p)",
-    class_name.c_str(), reinterpret_cast<void *>(new_factory));
+    class_name.c_str(), reinterpret_cast<void *>(new_factory.get()));
 }
 
 /**
@@ -278,22 +292,22 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla
  * @return A pointer to newly created plugin, note caller is responsible for object destruction
  */
 template<typename Base>
-Base * createInstance(const std::string & derived_class_name, ClassLoader * loader)
+Base * createInstance(const std::string & derived_class_name, std::shared_ptr<ClassLoader> loader)
 {
   AbstractMetaObject<Base> * factory = nullptr;
 
   getPluginBaseToFactoryMapMapMutex().lock();
   FactoryMap & factoryMap = getFactoryMapForBaseClass<Base>();
   if (factoryMap.find(derived_class_name) != factoryMap.end()) {
-    factory = dynamic_cast<impl::AbstractMetaObject<Base> *>(factoryMap[derived_class_name]);
+    factory = dynamic_cast<impl::AbstractMetaObject<Base> *>(factoryMap[derived_class_name].get());
   } else {
     CONSOLE_BRIDGE_logError(
       "class_loader.impl: No metaobject exists for class type %s.", derived_class_name.c_str());
   }
   getPluginBaseToFactoryMapMapMutex().unlock();
 
   Base * obj = nullptr;
-  if (factory != nullptr && factory->isOwnedBy(loader)) {
+  if (factory != nullptr && factory->isOwnedBy(loader.get())) {
     obj = factory->create();
   }
 
@@ -333,7 +347,7 @@ Base * createInstance(const std::string & derived_class_name, ClassLoader * load
  * @return A vector of strings where each string is a plugin we can create
  */
 template<typename Base>
-std::vector<std::string> getAvailableClasses(const ClassLoader * loader)
+std::vector<std::string> getAvailableClasses(std::shared_ptr<const ClassLoader> loader)
 {
   std::lock_guard<std::recursive_mutex> lock(getPluginBaseToFactoryMapMapMutex());
 
@@ -342,8 +356,8 @@ std::vector<std::string> getAvailableClasses(const ClassLoader * loader)
   std::vector<std::string> classes_with_no_owner;
 
   for (auto & it : factory_map) {
-    AbstractMetaObjectBase * factory = it.second;
-    if (factory->isOwnedBy(loader)) {
+    auto factory = it.second;
+    if (factory->isOwnedBy(loader.get())) {
       classes.push_back(it.first);
     } else if (factory->isOwnedBy(nullptr)) {
       classes_with_no_owner.push_back(it.first);
@@ -364,7 +378,8 @@ std::vector<std::string> getAvailableClasses(const ClassLoader * loader)
  *   within a ClassLoader's visible scope
  */
 CLASS_LOADER_PUBLIC
-std::vector<std::string> getAllLibrariesUsedByClassLoader(const ClassLoader * loader);
+std::vector<std::string> getAllLibrariesUsedByClassLoader(
+  std::shared_ptr<const ClassLoader> loader);
 
 /**
  * @brief Indicates if passed library loaded within scope of a ClassLoader.
@@ -375,7 +390,7 @@ std::vector<std::string> getAllLibrariesUsedByClassLoader(const ClassLoader * lo
  * @return true if the library is loaded within loader's scope, else false
  */
 CLASS_LOADER_PUBLIC
-bool isLibraryLoaded(const std::string & library_path, const ClassLoader * loader);
+bool isLibraryLoaded(const std::string & library_path, std::shared_ptr<const ClassLoader> loader);
 
 /**
  * @brief Indicates if passed library has been loaded by ANY ClassLoader

include/class_loader/multi_library_class_loader.hpp

@@ -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;
 
@@ -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<ClassLoader> loader = getClassLoaderForClass<Base>(class_name);
     if (nullptr == loader) {
       throw class_loader::CreateClassException(
               "MultiLibraryClassLoader: Could not create object of class type " +
@@ -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<ClassLoader> loader = getClassLoaderForLibrary(library_path);
     if (nullptr == loader) {
       throw class_loader::NoClassLoaderExistsException(
               "Could not create instance as there is no ClassLoader in "
@@ -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<ClassLoader> loader = getClassLoaderForClass<Base>(class_name);
     if (nullptr == loader) {
       throw class_loader::CreateClassException(
               "MultiLibraryClassLoader: Could not create object of class type " + class_name +
@@ -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<ClassLoader> loader = getClassLoaderForLibrary(library_path);
     if (nullptr == loader) {
       throw class_loader::NoClassLoaderExistsException(
               "Could not create instance as there is no ClassLoader in "
@@ -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<ClassLoader> loader = getClassLoaderForClass<Base>(class_name);
     if (nullptr == loader) {
       throw class_loader::CreateClassException(
               "MultiLibraryClassLoader: Could not create class of type " + class_name);
@@ -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<ClassLoader> loader = getClassLoaderForLibrary(library_path);
     if (nullptr == loader) {
       throw class_loader::NoClassLoaderExistsException(
               "Could not create instance as there is no ClassLoader in MultiLibraryClassLoader "
@@ -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<ClassLoader> loader = getClassLoaderForLibrary(library_path);
     if (nullptr == loader) {
       throw class_loader::NoClassLoaderExistsException(
               "There is no ClassLoader in MultiLibraryClassLoader bound to library " +
@@ -321,15 +321,15 @@ 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<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<ClassLoader> getClassLoaderForClass(const std::string & class_name)
   {
     ClassLoaderVector loaders = getAllAvailableClassLoaders();
     for (ClassLoaderVector::iterator i = loaders.begin(); i != loaders.end(); ++i) {
@@ -355,7 +355,7 @@ class CLASS_LOADER_PUBLIC MultiLibraryClassLoader
    */
   void shutdownAllClassLoaders();
 
-  MultiLibraryClassLoaderImpl * impl_;
+  std::unique_ptr<MultiLibraryClassLoaderImpl> impl_;
 };
 
 

src/class_loader.cpp

@@ -29,13 +29,22 @@
 
 #include "class_loader/class_loader.hpp"
 
+#include <memory>
 #include <string>
 
 namespace class_loader
 {
 
 bool ClassLoader::has_unmananged_instance_been_created_ = false;
 
+std::shared_ptr<ClassLoader> ClassLoader::Make(
+  const std::string & library_path,
+  bool ondemand_load_unload)
+{
+  // Not using make_shared because the constructor for ClassLoader is private
+  return std::shared_ptr<ClassLoader>(new ClassLoader{library_path, ondemand_load_unload});
+}
+
 bool ClassLoader::hasUnmanagedInstanceBeenCreated()
 {
   return ClassLoader::has_unmananged_instance_been_created_;
@@ -82,7 +91,7 @@ const std::string & ClassLoader::getLibraryPath() const
 
 bool ClassLoader::isLibraryLoaded() const
 {
-  return class_loader::impl::isLibraryLoaded(getLibraryPath(), this);
+  return class_loader::impl::isLibraryLoaded(getLibraryPath(), shared_from_this());
 }
 
 bool ClassLoader::isLibraryLoadedByAnyClassloader() const