From 35a23ddb4aaf96bcae431fa59f365e3e85634135 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Sun, 25 Sep 2022 00:42:53 +0800 Subject: [PATCH 1/8] make the meta-object alive only in the lifecycle of the registered plugin Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 58 ++++++++++++++++++++-- include/class_loader/register_macro.hpp | 4 +- src/class_loader_core.cpp | 51 ++++++------------- test/unique_ptr_test.cpp | 28 +++++++++++ 4 files changed, 99 insertions(+), 42 deletions(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index bfb17fe3..cc661ca0 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -81,6 +82,16 @@ typedef std::map BaseToFactoryMapMap; typedef std::pair> LibraryPair; typedef std::vector LibraryVector; typedef std::vector MetaObjectVector; +class MetaObjectGraveyardVector : public MetaObjectVector +{ +public: + ~MetaObjectGraveyardVector() + { + // make sure to destroy `meta_object` not to access the pointer value in the class loader plugin + // that could be unloaded before `g_register_plugin_ ## UniqueID` in some circumstances. + clear(); + } +}; CLASS_LOADER_PUBLIC void printDebugInfoToScreen(); @@ -164,6 +175,13 @@ 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. + */ +MetaObjectGraveyardVector & getMetaObjectGraveyard(); + /** * @brief To provide thread safety, all exposed plugin functions can only be run serially by multiple threads. * @@ -176,6 +194,8 @@ CLASS_LOADER_PUBLIC std::recursive_mutex & getLoadedLibraryVectorMutex(); CLASS_LOADER_PUBLIC std::recursive_mutex & getPluginBaseToFactoryMapMapMutex(); +CLASS_LOADER_PUBLIC +std::mutex & getMetaObjectGraveyardMutex(); /** * @brief Indicates if a library containing more than just plugins has been opened by the running process @@ -193,6 +213,12 @@ bool hasANonPurePluginLibraryBeenOpened(); CLASS_LOADER_PUBLIC void hasANonPurePluginLibraryBeenOpened(bool hasIt); +template +using DeleterType = std::function; + +template +using UniquePtr = std::unique_ptr>; + // Plugin Functions /** @@ -207,7 +233,8 @@ void hasANonPurePluginLibraryBeenOpened(bool hasIt); * @param class_name - the literal name of the class being registered (NOT MANGLED) */ template -void registerPlugin(const std::string & class_name, const std::string & base_class_name) +UniquePtr +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 +267,28 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla } // Create factory - impl::AbstractMetaObject * new_factory = - new impl::MetaObject(class_name, base_class_name); + UniquePtr new_factory( + new impl::MetaObject(class_name, base_class_name), + [class_name](AbstractMetaObjectBase * p) { + getMetaObjectGraveyardMutex().lock(); + MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard(); + for (auto iter = graveyard.begin(); iter != graveyard.end(); ++iter) { + if (*iter == p) { + graveyard.erase(iter); + break; + } + } + getMetaObjectGraveyardMutex().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 +307,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(new_factory)); + class_name.c_str(), reinterpret_cast(new_factory.get())); + return new_factory; } /** diff --git a/include/class_loader/register_macro.hpp b/include/class_loader/register_macro.hpp index 77207b4e..c39a1dd8 100644 --- 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 holder; \ }; \ static ProxyExec ## UniqueID g_register_plugin_ ## UniqueID; \ } // namespace diff --git a/src/class_loader_core.cpp b/src/class_loader_core.cpp index 56cc7704..cf7781a5 100644 --- a/src/class_loader_core.cpp +++ b/src/class_loader_core.cpp @@ -55,6 +55,12 @@ std::recursive_mutex & getPluginBaseToFactoryMapMapMutex() return m; } +std::mutex & getMetaObjectGraveyardMutex() +{ + static std::mutex m; + return m; +} + BaseToFactoryMapMap & getGlobalPluginBaseToFactoryMapMap() { static BaseToFactoryMapMap instance; @@ -72,9 +78,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; } @@ -214,7 +220,9 @@ void insertMetaObjectIntoGraveyard(AbstractMetaObjectBase * meta_obj) "Inserting MetaObject (class = %s, base_class = %s, ptr = %p) into graveyard", meta_obj->className().c_str(), meta_obj->baseClassName().c_str(), reinterpret_cast(meta_obj)); + getMetaObjectGraveyardMutex().lock(); getMetaObjectGraveyard().push_back(meta_obj); + getMetaObjectGraveyardMutex().unlock(); } void destroyMetaObjectsForLibrary( @@ -352,7 +360,7 @@ void revivePreviouslyCreateMetaobjectsFromGraveyard( const std::string & library_path, ClassLoader * loader) { std::lock_guard b2fmm_lock(getPluginBaseToFactoryMapMapMutex()); - MetaObjectVector & graveyard = getMetaObjectGraveyard(); + MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard(); for (auto & obj : graveyard) { if (obj->getAssociatedLibraryPath() == library_path) { @@ -373,14 +381,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 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 +401,7 @@ void purgeGraveyardOfMetaobjects( reinterpret_cast(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(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 +465,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 llv_lock(getLoadedLibraryVectorMutex()); diff --git a/test/unique_ptr_test.cpp b/test/unique_ptr_test.cpp index 9d907a12..35d13c62 100644 --- 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("Cat")->saySomething(); // See if lazy load works + } + ClassLoader loader1(LIBRARY_1, false); + loader1.createUniqueInstance("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("Cat")->saySomething(); // See if lazy load works + ClassLoader loader2(LIBRARY_1, false); + loader2.createUniqueInstance("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); From de7241a6ff367ba5859ceadea6a8cca658e410af Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Mon, 26 Sep 2022 10:23:30 +0800 Subject: [PATCH 2/8] remove unuseful variable passed in the lambda Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index cc661ca0..78949800 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -269,7 +269,7 @@ registerPlugin(const std::string & class_name, const std::string & base_class_na // Create factory UniquePtr new_factory( new impl::MetaObject(class_name, base_class_name), - [class_name](AbstractMetaObjectBase * p) { + [](AbstractMetaObjectBase * p) { getMetaObjectGraveyardMutex().lock(); MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard(); for (auto iter = graveyard.begin(); iter != graveyard.end(); ++iter) { From 2eb0eb772c4171e7ae6893f15160a894b402a592 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Mon, 26 Sep 2022 16:58:47 +0800 Subject: [PATCH 3/8] add extra tests to make test cases exists in both unique ptr test and utest Signed-off-by: Chen Lihui --- test/utest.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/utest.cpp b/test/utest.cpp index 6a0870b5..b80817a6 100644 --- 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("Cat")->saySomething(); // See if lazy load works + } + class_loader::ClassLoader loader1(LIBRARY_1, false); + ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen()); + loader1.createInstance("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("Cat")->saySomething(); // See if lazy load works + class_loader::ClassLoader loader2(LIBRARY_1, false); + ASSERT_NO_THROW(class_loader::impl::printDebugInfoToScreen()); + loader2.createInstance("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 { From 1cc61a553c1db1dd322e60651e2a78af0f6031f3 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Tue, 27 Sep 2022 09:45:48 +0800 Subject: [PATCH 4/8] use the original mutex rather than import a new mutex Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 6 ++---- src/class_loader_core.cpp | 8 -------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index 78949800..3bb773eb 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -194,8 +194,6 @@ CLASS_LOADER_PUBLIC std::recursive_mutex & getLoadedLibraryVectorMutex(); CLASS_LOADER_PUBLIC std::recursive_mutex & getPluginBaseToFactoryMapMapMutex(); -CLASS_LOADER_PUBLIC -std::mutex & getMetaObjectGraveyardMutex(); /** * @brief Indicates if a library containing more than just plugins has been opened by the running process @@ -270,7 +268,7 @@ registerPlugin(const std::string & class_name, const std::string & base_class_na UniquePtr new_factory( new impl::MetaObject(class_name, base_class_name), [](AbstractMetaObjectBase * p) { - getMetaObjectGraveyardMutex().lock(); + getPluginBaseToFactoryMapMapMutex().lock(); MetaObjectGraveyardVector & graveyard = getMetaObjectGraveyard(); for (auto iter = graveyard.begin(); iter != graveyard.end(); ++iter) { if (*iter == p) { @@ -278,7 +276,7 @@ registerPlugin(const std::string & class_name, const std::string & base_class_na break; } } - getMetaObjectGraveyardMutex().unlock(); + getPluginBaseToFactoryMapMapMutex().unlock(); #ifndef _WIN32 #pragma GCC diagnostic push diff --git a/src/class_loader_core.cpp b/src/class_loader_core.cpp index cf7781a5..e5458a55 100644 --- a/src/class_loader_core.cpp +++ b/src/class_loader_core.cpp @@ -55,12 +55,6 @@ std::recursive_mutex & getPluginBaseToFactoryMapMapMutex() return m; } -std::mutex & getMetaObjectGraveyardMutex() -{ - static std::mutex m; - return m; -} - BaseToFactoryMapMap & getGlobalPluginBaseToFactoryMapMap() { static BaseToFactoryMapMap instance; @@ -220,9 +214,7 @@ void insertMetaObjectIntoGraveyard(AbstractMetaObjectBase * meta_obj) "Inserting MetaObject (class = %s, base_class = %s, ptr = %p) into graveyard", meta_obj->className().c_str(), meta_obj->baseClassName().c_str(), reinterpret_cast(meta_obj)); - getMetaObjectGraveyardMutex().lock(); getMetaObjectGraveyard().push_back(meta_obj); - getMetaObjectGraveyardMutex().unlock(); } void destroyMetaObjectsForLibrary( From 6ef26d53eca5335eea76876dfb60914e48abe9a4 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Tue, 27 Sep 2022 11:27:44 +0800 Subject: [PATCH 5/8] update document comment Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index 3bb773eb..df2399e3 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -87,8 +87,10 @@ class MetaObjectGraveyardVector : public MetaObjectVector public: ~MetaObjectGraveyardVector() { - // make sure to destroy `meta_object` not to access the pointer value in the class loader plugin - // that could be unloaded before `g_register_plugin_ ## UniqueID` in some circumstances. + // 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()` could be finalized before the static variable + // `g_register_plugin_ ## UniqueID` in some circumstances. clear(); } }; From 57ef60886463ec6b4992e8397004874c5dc087f6 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Tue, 27 Sep 2022 12:40:37 +0800 Subject: [PATCH 6/8] symbol visibility for Windows Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index df2399e3..8d62c80d 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -182,6 +182,7 @@ FactoryMap & getFactoryMapForBaseClass() * * @return A reference to the MetaObjectGraveyardVector contained within the meta object of graveyard. */ +CLASS_LOADER_PUBLIC MetaObjectGraveyardVector & getMetaObjectGraveyard(); /** From 02e1a8a0ed1cd1687831f234490f3b2697f71f80 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Tue, 27 Sep 2022 14:40:06 +0800 Subject: [PATCH 7/8] add extra document comment Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index 8d62c80d..667559d1 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -89,8 +89,10 @@ class MetaObjectGraveyardVector : public MetaObjectVector { // 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()` could be finalized before the static variable + // `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(); } }; From c44e59e7cc808f3461fb051f8d4d281625a0077a Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Tue, 27 Sep 2022 16:20:39 +0800 Subject: [PATCH 8/8] update doc Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index 667559d1..aa969c9e 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -234,6 +234,7 @@ using UniquePtr = std::unique_ptr>; * @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 to newly created meta object. */ template UniquePtr