make the meta-object alive in the lifecycle of the registered plugin (#…

…201) * make the meta-object alive only in the lifecycle of the registered plugin Signed-off-by: Chen Lihui <[email protected]> * remove unuseful variable passed in the lambda Signed-off-by: Chen Lihui <[email protected]> * add extra tests to make test cases exists in both unique ptr test and utest Signed-off-by: Chen Lihui <[email protected]> * use the original mutex rather than import a new mutex Signed-off-by: Chen Lihui <[email protected]> * update document comment Signed-off-by: Chen Lihui <[email protected]> * symbol visibility for Windows Signed-off-by: Chen Lihui <[email protected]> * add extra document comment Signed-off-by: Chen Lihui <[email protected]> * update doc Signed-off-by: Chen Lihui <[email protected]> Signed-off-by: Chen Lihui <[email protected]>
ros · Oct 3, 2022 · c427a50 · c427a50
1 parent 6ba5c47
commit c427a50
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 42 deletions.
diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp
@@ -34,6 +34,7 @@
 
 #include <cstddef>
 #include <cstdio>
+#include <functional>
 #include <map>
 #include <memory>
 #include <mutex>
@@ -81,6 +82,20 @@ 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;
+class MetaObjectGraveyardVector : public MetaObjectVector
+{
+public:
+  ~MetaObjectGraveyardVector()
+  {
+    // Make sure not to access the pointer value in the static variable of `getMetaObjectGraveyard()
+    // when destroying `meta_object` in the unique_ptr deleter. Because the static variable in
+    // `getMetaObjectGraveyard()` can be destructed before the static variable
+    // `g_register_plugin_ ## UniqueID` in some circumstances.
+    // NOTE of the vector dtor in the STL: if the elements themselves are pointers, the pointed-to
+    // memory is not touched in any way.  Managing the pointer is the user's responsibility.
+    clear();
+  }
+};
 
 CLASS_LOADER_PUBLIC
 void printDebugInfoToScreen();
@@ -164,6 +179,14 @@ FactoryMap & getFactoryMapForBaseClass()
   return getFactoryMapForBaseClass(typeid(Base).name());
 }
 
+/**
+ * @brief Gets a handle to a list of meta object of graveyard.
+ *
+ * @return A reference to the MetaObjectGraveyardVector contained within the meta object of graveyard.
+ */
+CLASS_LOADER_PUBLIC
+MetaObjectGraveyardVector & getMetaObjectGraveyard();
+
 /**
  * @brief To provide thread safety, all exposed plugin functions can only be run serially by multiple threads.
  *
@@ -193,6 +216,12 @@ bool hasANonPurePluginLibraryBeenOpened();
 CLASS_LOADER_PUBLIC
 void hasANonPurePluginLibraryBeenOpened(bool hasIt);
 
+template<typename Base>
+using DeleterType = std::function<void (Base *)>;
+
+template<typename Base>
+using UniquePtr = std::unique_ptr<Base, DeleterType<Base>>;
+
 // Plugin Functions
 
 /**
@@ -205,9 +234,11 @@ void hasANonPurePluginLibraryBeenOpened(bool hasIt);
  * @param Derived - parameteric type indicating concrete type of plugin
  * @param Base - parameteric type indicating base type of plugin
  * @param class_name - the literal name of the class being registered (NOT MANGLED)
+ * @return A class_loader::impl::UniquePtr<AbstractMetaObjectBase> to newly created meta object.
  */
 template<typename Derived, typename Base>
-void registerPlugin(const std::string & class_name, const std::string & base_class_name)
+UniquePtr<AbstractMetaObjectBase>
+registerPlugin(const std::string & class_name, const std::string & base_class_name)
 {
   // Note: This function will be automatically invoked when a dlopen() call
   // opens a library. Normally it will happen within the scope of loadLibrary(),
@@ -240,8 +271,28 @@ 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);
+  UniquePtr<AbstractMetaObjectBase> new_factory(
+    new impl::MetaObject<Derived, Base>(class_name, base_class_name),
+    [](AbstractMetaObjectBase * p) {
+      getPluginBaseToFactoryMapMapMutex().lock();
+      MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard();
+      for (auto iter = graveyard.begin(); iter != graveyard.end(); ++iter) {
+        if (*iter == p) {
+          graveyard.erase(iter);
+          break;
+        }
+      }
+      getPluginBaseToFactoryMapMapMutex().unlock();
+
+#ifndef _WIN32
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor"
+#endif
+      delete (p);  // Note: This is the only place where metaobjects can be destroyed
+#ifndef _WIN32
+#pragma GCC diagnostic pop
+#endif
+    });
   new_factory->addOwningClassLoader(getCurrentlyActiveClassLoader());
   new_factory->setAssociatedLibraryPath(getCurrentlyLoadingLibraryName());
 
@@ -260,13 +311,14 @@ 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());
   }
-  factoryMap[class_name] = new_factory;
+  factoryMap[class_name] = new_factory.get();
   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()));
+  return new_factory;
 }
 
 /**

diff --git a/include/class_loader/register_macro.hpp b/include/class_loader/register_macro.hpp
@@ -56,8 +56,10 @@
       if (!std::string(Message).empty()) { \
         CONSOLE_BRIDGE_logInform("%s", Message); \
       } \
-      class_loader::impl::registerPlugin<_derived, _base>(#Derived, #Base); \
+      holder = class_loader::impl::registerPlugin<_derived, _base>(#Derived, #Base); \
     } \
+private: \
+    class_loader::impl::UniquePtr<class_loader::impl::AbstractMetaObjectBase> holder; \
   }; \
   static ProxyExec ## UniqueID g_register_plugin_ ## UniqueID; \
   }  // namespace

diff --git a/src/class_loader_core.cpp b/src/class_loader_core.cpp
@@ -72,9 +72,9 @@ FactoryMap & getFactoryMapForBaseClass(const std::string & typeid_base_class_nam
   return factoryMapMap[base_class_name];
 }
 
-MetaObjectVector & getMetaObjectGraveyard()
+MetaObjectGraveyardVector & getMetaObjectGraveyard()
 {
-  static MetaObjectVector instance;
+  static MetaObjectGraveyardVector instance;
   return instance;
 }
 
@@ -352,7 +352,7 @@ void revivePreviouslyCreateMetaobjectsFromGraveyard(
   const std::string & library_path, ClassLoader * loader)
 {
   std::lock_guard<std::recursive_mutex> b2fmm_lock(getPluginBaseToFactoryMapMapMutex());
-  MetaObjectVector & graveyard = getMetaObjectGraveyard();
+  MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard();
 
   for (auto & obj : graveyard) {
     if (obj->getAssociatedLibraryPath() == library_path) {
@@ -373,14 +373,14 @@ void revivePreviouslyCreateMetaobjectsFromGraveyard(
 }
 
 void purgeGraveyardOfMetaobjects(
-  const std::string & library_path, ClassLoader * loader, bool delete_objs)
+  const std::string & library_path, ClassLoader * loader)
 {
   MetaObjectVector all_meta_objs = allMetaObjects();
   // Note: Lock must happen after call to allMetaObjects as that will lock
   std::lock_guard<std::recursive_mutex> b2fmm_lock(getPluginBaseToFactoryMapMapMutex());
 
-  MetaObjectVector & graveyard = getMetaObjectGraveyard();
-  MetaObjectVector::iterator itr = graveyard.begin();
+  MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard();
+  MetaObjectGraveyardVector::iterator itr = graveyard.begin();
 
   while (itr != graveyard.end()) {
     AbstractMetaObjectBase * obj = *itr;
@@ -393,34 +393,7 @@ void purgeGraveyardOfMetaobjects(
         reinterpret_cast<void *>(loader),
         nullptr == loader ? "NULL" : loader->getLibraryPath().c_str());
 
-      bool is_address_in_graveyard_same_as_global_factory_map =
-        std::find(all_meta_objs.begin(), all_meta_objs.end(), *itr) != all_meta_objs.end();
       itr = graveyard.erase(itr);
-      if (delete_objs) {
-        if (is_address_in_graveyard_same_as_global_factory_map) {
-          CONSOLE_BRIDGE_logDebug(
-            "%s",
-            "class_loader.impl: "
-            "Newly created metaobject factory in global factory map map has same address as "
-            "one in graveyard -- metaobject has been purged from graveyard but not deleted.");
-        } else {
-          assert(hasANonPurePluginLibraryBeenOpened() == false);
-          CONSOLE_BRIDGE_logDebug(
-            "class_loader.impl: "
-            "Also destroying metaobject %p (class = %s, base_class = %s, library_path = %s) "
-            "in addition to purging it from graveyard.",
-            reinterpret_cast<void *>(obj), obj->className().c_str(), obj->baseClassName().c_str(),
-            obj->getAssociatedLibraryPath().c_str());
-#ifndef _WIN32
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor"
-#endif
-          delete (obj);  // Note: This is the only place where metaobjects can be destroyed
-#ifndef _WIN32
-#pragma GCC diagnostic pop
-#endif
-        }
-      }
     } else {
       itr++;
     }
@@ -484,16 +457,14 @@ void loadLibrary(const std::string & library_path, ClassLoader * loader)
       "Checking factory graveyard for previously loaded metaobjects...",
       library_path.c_str());
     revivePreviouslyCreateMetaobjectsFromGraveyard(library_path, loader);
-    // Note: The 'false' indicates we don't want to invoke delete on the metaobject
-    purgeGraveyardOfMetaobjects(library_path, loader, false);
   } else {
     CONSOLE_BRIDGE_logDebug(
       "class_loader.impl: "
       "Library %s generated new factory metaobjects on load. "
       "Destroying graveyarded objects from previous loads...",
       library_path.c_str());
-    purgeGraveyardOfMetaobjects(library_path, loader, true);
   }
+  purgeGraveyardOfMetaobjects(library_path, loader);
 
   // Insert library into global loaded library vectory
   std::lock_guard<std::recursive_mutex> llv_lock(getLoadedLibraryVectorMutex());

diff --git a/test/unique_ptr_test.cpp b/test/unique_ptr_test.cpp
@@ -59,6 +59,34 @@ TEST(ClassLoaderUniquePtrTest, basicLoad) {
   }
 }
 
+TEST(ClassLoaderUniquePtrTest, basicLoadTwice) {
+  try {
+    {
+      ClassLoader loader1(LIBRARY_1, false);
+      loader1.createUniqueInstance<Base>("Cat")->saySomething();  // See if lazy load works
+    }
+    ClassLoader loader1(LIBRARY_1, false);
+    loader1.createUniqueInstance<Base>("Cat")->saySomething();  // See if lazy load works
+    ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen());
+    SUCCEED();
+  } catch (class_loader::ClassLoaderException & e) {
+    FAIL() << "ClassLoaderException: " << e.what() << "\n";
+  }
+}
+
+TEST(ClassLoaderUniquePtrTest, basicLoadTwiceSameTime) {
+  try {
+    ClassLoader loader1(LIBRARY_1, false);
+    loader1.createUniqueInstance<Base>("Cat")->saySomething();  // See if lazy load works
+    ClassLoader loader2(LIBRARY_1, false);
+    loader2.createUniqueInstance<Base>("Cat")->saySomething();  // See if lazy load works
+    ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen());
+    SUCCEED();
+  } catch (class_loader::ClassLoaderException & e) {
+    FAIL() << "ClassLoaderException: " << e.what() << "\n";
+  }
+}
+
 TEST(ClassLoaderUniquePtrTest, basicLoadFailures) {
   ClassLoader loader1(LIBRARY_1, false);
   ClassLoader loader2("", false);

diff --git a/test/utest.cpp b/test/utest.cpp
@@ -67,6 +67,38 @@ TEST(ClassLoaderTest, basicLoad) {
   SUCCEED();
 }
 
+TEST(ClassLoaderTest, basicLoadTwice) {
+  try {
+    {
+      class_loader::ClassLoader loader1(LIBRARY_1, false);
+      ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen());
+      loader1.createInstance<Base>("Cat")->saySomething();  // See if lazy load works
+    }
+    class_loader::ClassLoader loader1(LIBRARY_1, false);
+    ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen());
+    loader1.createInstance<Base>("Cat")->saySomething();  // See if lazy load works
+  } catch (class_loader::ClassLoaderException & e) {
+    FAIL() << "ClassLoaderException: " << e.what() << "\n";
+  }
+
+  SUCCEED();
+}
+
+TEST(ClassLoaderTest, basicLoadTwiceSameTime) {
+  try {
+    class_loader::ClassLoader loader1(LIBRARY_1, false);
+    ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen());
+    loader1.createInstance<Base>("Cat")->saySomething();  // See if lazy load works
+    class_loader::ClassLoader loader2(LIBRARY_1, false);
+    ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen());
+    loader2.createInstance<Base>("Cat")->saySomething();  // See if lazy load works
+  } catch (class_loader::ClassLoaderException & e) {
+    FAIL() << "ClassLoaderException: " << e.what() << "\n";
+  }
+
+  SUCCEED();
+}
+
 // Requires separate namespace so static variables are isolated
 TEST(ClassLoaderUnmanagedTest, basicLoadUnmanaged) {
   try {