From c427a50f0e304d61a8264252547909c0136dae61 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Tue, 4 Oct 2022 06:55:46 +0800 Subject: [PATCH] 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 * remove unuseful variable passed in the lambda Signed-off-by: Chen Lihui * add extra tests to make test cases exists in both unique ptr test and utest Signed-off-by: Chen Lihui * use the original mutex rather than import a new mutex Signed-off-by: Chen Lihui * update document comment Signed-off-by: Chen Lihui * symbol visibility for Windows Signed-off-by: Chen Lihui * add extra document comment Signed-off-by: Chen Lihui * update doc Signed-off-by: Chen Lihui Signed-off-by: Chen Lihui --- include/class_loader/class_loader_core.hpp | 62 ++++++++++++++++++++-- include/class_loader/register_macro.hpp | 4 +- src/class_loader_core.cpp | 43 +++------------ test/unique_ptr_test.cpp | 28 ++++++++++ test/utest.cpp | 32 +++++++++++ 5 files changed, 127 insertions(+), 42 deletions(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index bfb17fe3..aa969c9e 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,20 @@ typedef std::map BaseToFactoryMapMap; typedef std::pair> LibraryPair; typedef std::vector LibraryVector; typedef std::vector 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 +using DeleterType = std::function; + +template +using UniquePtr = std::unique_ptr>; + // 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 to newly created meta object. */ 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 +271,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), + [](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(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..e5458a55 100644 --- 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 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 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(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 +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 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); 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 {