diff --git a/include/class_loader/class_loader.hpp b/include/class_loader/class_loader.hpp index 27da8cf..d9ada57 100644 --- a/include/class_loader/class_loader.hpp +++ b/include/class_loader/class_loader.hpp @@ -275,7 +275,12 @@ class ClassLoader if (context->loader) { context->loader->onPluginDeletion(p); } else { - delete p; // The library is still loaded: it is not unloaded while instances exist. + // The ClassLoader is gone, but its handle in context->library kept the library + // loaded. The last instance closes it. + delete p; + if (context->orphaned_instances > 0 && --context->orphaned_instances == 0) { + context->library.reset(); + } } }; } @@ -400,6 +405,10 @@ class ClassLoader { std::recursive_mutex mutex; ClassLoader * loader = nullptr; // Cleared when the ClassLoader is destroyed. + // Set when the ClassLoader is destroyed while managed instances still exist: a handle of + // their own that keeps the library loaded, and how many instances still need it. + std::shared_ptr library; + std::size_t orphaned_instances = 0; }; std::shared_ptr deleter_context_; }; diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index 418418b..2548804 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -355,12 +355,16 @@ Base * createInstance(const std::string & derived_class_name, ClassLoader * load CONSOLE_BRIDGE_logError( "class_loader.impl: No metaobject exists for class type %s.", derived_class_name.c_str()); } + // Check ownership under the lock (other threads change it), but construct outside it. + const bool owned_by_loader = factory != nullptr && factory->isOwnedBy(loader); + const bool owned_by_nobody = factory != nullptr && !owned_by_loader && + factory->isOwnedBy(nullptr); getPluginBaseToFactoryMapMapMutex().unlock(); Base * obj = nullptr; - if (factory != nullptr && factory->isOwnedBy(loader)) { + if (owned_by_loader) { obj = factory->create(std::forward(args)...); - } else if (factory && factory->isOwnedBy(nullptr)) { + } else if (owned_by_nobody) { CONSOLE_BRIDGE_logDebug( "%s", "class_loader.impl: ALERT!!! " diff --git a/src/class_loader.cpp b/src/class_loader.cpp index 66d98a8..c759f95 100644 --- a/src/class_loader.cpp +++ b/src/class_loader.cpp @@ -75,17 +75,41 @@ ClassLoader::~ClassLoader() "class_loader.ClassLoader: Destroying class loader, " "unloading associated library...\n"); std::lock_guard lock(deleter_context_->mutex); - { - // The deferred on-demand unload in onPluginDeletion() can't happen once this loader is gone. - std::lock_guard ref_count_lock(plugin_ref_count_mutex_); - if (plugin_ref_count_ > 0 && isOnDemandLoadUnloadEnabled()) { - CONSOLE_BRIDGE_logWarn( - "%s", - "class_loader.ClassLoader: SEVERE WARNING!!! " - "Destroying ClassLoader while objects created by it exist in the heap!"); + std::lock_guard ref_count_lock(plugin_ref_count_mutex_); + std::lock_guard load_ref_lock(load_ref_count_mutex_); + + if (getLibraryPath().empty() || load_ref_count_ <= 0) { + deleter_context_->loader = nullptr; + return; + } + + if (plugin_ref_count_ > 0) { + // Managed instances outlive this loader. Open another handle to the library for them, so it + // stays loaded until the last of them is deleted (see makeDeleter()), even once nothing else + // uses it. + try { + deleter_context_->library = std::make_shared(getLibraryPath()); + deleter_context_->orphaned_instances = static_cast(plugin_ref_count_); + } catch (const std::exception & e) { + CONSOLE_BRIDGE_logError( + "class_loader.ClassLoader: Destroying ClassLoader for %s while objects created by it " + "exist, and could not keep the library loaded for them (%s). The library will stay " + "loaded.", getLibraryPath().c_str(), e.what()); + deleter_context_->loader = nullptr; + return; } } - unloadLibrary(); // TODO(mikaelarguedas): while(unloadLibrary() > 0){} ?? + + // Unbind from the library however many times loadLibrary() was called. Leaving this loader + // registered as an owner of the library's classes would leave a dangling pointer behind, and a + // later ClassLoader at the same address would appear to own them. + try { + class_loader::impl::unloadLibrary(getLibraryPath(), this); + } catch (const class_loader::LibraryUnloadException & e) { + CONSOLE_BRIDGE_logError( + "class_loader.ClassLoader: Could not unload %s: %s", getLibraryPath().c_str(), e.what()); + } + load_ref_count_ = 0; deleter_context_->loader = nullptr; } diff --git a/src/class_loader_core.cpp b/src/class_loader_core.cpp index 6c08c37..0a64eb1 100644 --- a/src/class_loader_core.cpp +++ b/src/class_loader_core.cpp @@ -195,18 +195,24 @@ filterAllMetaObjectsAssociatedWithLibrary( MetaObjectVector allMetaObjectsForClassLoader(const ClassLoader * owner) { + // Filter under the lock too: other threads change owners and destroy metaobjects under it. + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); return filterAllMetaObjectsOwnedBy(allMetaObjects(), owner); } MetaObjectVector allMetaObjectsForLibrary(const std::string & library_path) { + // Filter under the lock too: other threads change owners and destroy metaobjects under it. + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); return filterAllMetaObjectsAssociatedWithLibrary(allMetaObjects(), library_path); } MetaObjectVector allMetaObjectsForLibraryOwnedBy(const std::string & library_path, const ClassLoader * owner) { + // Filter under the lock too: other threads change owners and destroy metaobjects under it. + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); return filterAllMetaObjectsOwnedBy(allMetaObjectsForLibrary(library_path), owner); } @@ -302,6 +308,9 @@ bool isLibraryLoadedByAnybody(const std::string & library_path) bool isLibraryLoaded(const std::string & library_path, const ClassLoader * loader) { + // Answer from one consistent state, in the same lock order as unloadLibrary(). + std::lock_guard llv_lock(getLoadedLibraryVectorMutex()); + std::lock_guard b2fmm_lock(getPluginBaseToFactoryMapMapMutex()); bool is_lib_loaded_by_anyone = isLibraryLoadedByAnybody(library_path); size_t num_meta_objs_for_lib = allMetaObjectsForLibrary(library_path).size(); size_t num_meta_objs_for_lib_bound_to_loader = diff --git a/test/utest.cpp b/test/utest.cpp index e9c3da1..3c9a608 100644 --- a/test/utest.cpp +++ b/test/utest.cpp @@ -33,7 +33,10 @@ #include #endif +#include +#include #include +#include #include #include #include @@ -387,8 +390,164 @@ TEST(ClassLoaderBasicTest, isLibraryLoadedScopedToLoader) { ASSERT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); } +#ifndef _WIN32 +// Whether the dynamic linker still has the library mapped, whatever class_loader thinks. +bool isLibraryMapped(const std::string & library_path) +{ + void * handle = dlopen(library_path.c_str(), RTLD_LAZY | RTLD_NOLOAD); + if (handle == nullptr) { + return false; + } + dlclose(handle); + return true; +} + +// Instances that outlive their ClassLoader keep the library loaded, and the last of them unloads +// it, in both on-demand and non-on-demand mode. +TEST(ClassLoaderLifetimeTest, lastOrphanedInstanceUnloadsLibrary) { + for (bool on_demand : {false, true}) { + SCOPED_TRACE(on_demand ? "on-demand" : "non-on-demand"); + ASSERT_FALSE(isLibraryMapped(LIBRARY_2)); + auto loader = std::make_unique(LIBRARY_2, on_demand); + std::shared_ptr shared = loader->createInstance("Robot"); + class_loader::ClassLoader::UniquePtr unique = loader->createUniqueInstance("Robot"); + loader.reset(); + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_2)); + EXPECT_TRUE(isLibraryMapped(LIBRARY_2)); + + shared.reset(); + EXPECT_EQ("Beep boop", unique->saySomething()); + EXPECT_TRUE(isLibraryMapped(LIBRARY_2)); + unique.reset(); + EXPECT_FALSE(isLibraryMapped(LIBRARY_2)); + } +} +#endif + +// The library can be loaded again while instances that outlived their ClassLoader still exist. +TEST(ClassLoaderLifetimeTest, reloadWhileOrphanedInstancesExist) { + auto loader = std::make_unique(LIBRARY_2, false); + std::shared_ptr orphan = loader->createInstance("Robot"); + loader.reset(); + + class_loader::ClassLoader loader2(LIBRARY_2, false); + EXPECT_EQ("Beep boop", loader2.createInstance("Robot")->saySomething()); + orphan.reset(); + EXPECT_TRUE(loader2.isLibraryLoaded()); + EXPECT_EQ("Beep boop", loader2.createUniqueInstance("Robot")->saySomething()); +} + +// A destroyed ClassLoader must not stay registered as an owner of its library's classes. If it +// did, a new ClassLoader at the same address could see and create them without loading anything. +TEST(ClassLoaderLifetimeTest, destroyedLoaderOwnsNothing) { + enum class Case { kLiveInstance, kLiveInstanceOnDemand, kLoadedTwice }; + for (Case c : {Case::kLiveInstance, Case::kLiveInstanceOnDemand, Case::kLoadedTwice}) { + SCOPED_TRACE(static_cast(c)); + alignas(class_loader::ClassLoader) unsigned char storage[sizeof(class_loader::ClassLoader)]; + std::shared_ptr dog; + { + auto * loader = new (storage) class_loader::ClassLoader( + LIBRARY_1, c == Case::kLiveInstanceOnDemand); + if (c == Case::kLoadedTwice) { + loader->loadLibrary(); + } else { + dog = loader->createInstance("Dog"); + } + loader->~ClassLoader(); + } + { + auto * other = new (storage) class_loader::ClassLoader(LIBRARY_2, true); + EXPECT_FALSE(other->isClassAvailable("Dog")); + other->~ClassLoader(); + } + dog.reset(); + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); + } +} + +// Stress tests for state shared between loaders. Mostly useful under ThreadSanitizer. +constexpr int STRESS_ITERATIONS = 20; +constexpr size_t STRESS_THREADS = 8; + +TEST(ClassLoaderStressTest, loadersForSameLibraryOnManyThreads) { + std::vector threads; + for (size_t t = 0; t < STRESS_THREADS; ++t) { + threads.emplace_back( + [t]() { + for (int i = 0; i < STRESS_ITERATIONS; ++i) { + class_loader::ClassLoader loader(LIBRARY_1, (t + i) % 2 == 0); + EXPECT_EQ("Bark", loader.createInstance("Dog")->saySomething()); + EXPECT_EQ("Meow", loader.createUniqueInstance("Cat")->saySomething()); + } + }); + } + for (auto & thread : threads) { + thread.join(); + } + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); +} + +TEST(ClassLoaderStressTest, multiLibraryClassLoadersOnManyThreads) { + std::vector threads; + for (size_t t = 0; t < STRESS_THREADS; ++t) { + threads.emplace_back( + [t]() { + for (int i = 0; i < STRESS_ITERATIONS; ++i) { + class_loader::MultiLibraryClassLoader loader((t + i) % 2 == 0); + loader.loadLibrary(LIBRARY_1); + loader.loadLibrary(LIBRARY_2); + if ((t + i) % 2 != 0) { + // An on-demand loader only lists the classes of libraries it has actually loaded. + const auto classes = loader.getAvailableClasses(); + EXPECT_NE(classes.end(), std::ranges::find(classes, "Dog")); + EXPECT_NE(classes.end(), std::ranges::find(classes, "Robot")); + } + EXPECT_EQ("Bark", loader.createInstance("Dog")->saySomething()); + EXPECT_EQ("Beep boop", loader.createUniqueInstance("Robot")->saySomething()); + loader.unloadLibrary(LIBRARY_1); + } + }); + } + for (auto & thread : threads) { + thread.join(); + } + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_2)); +} + +// Instances deleted on other threads while their ClassLoader is being destroyed. +TEST(ClassLoaderStressTest, instancesReleasedWhileLoaderDestroyed) { + for (int i = 0; i < STRESS_ITERATIONS; ++i) { + auto loader = std::make_unique(LIBRARY_1, i % 2 == 0); + std::vector> instances; + for (size_t t = 0; t < STRESS_THREADS; ++t) { + instances.push_back(loader->createInstance("Dog")); + } + std::atomic start{false}; + std::vector threads; + for (auto & instance : instances) { + threads.emplace_back( + [&start, instance = std::move(instance)]() mutable { + while (!start.load()) { + std::this_thread::yield(); + } + EXPECT_EQ("Bark", instance->saySomething()); + instance.reset(); + }); + } + start = true; + loader.reset(); + for (auto & thread : threads) { + thread.join(); + } + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); + } +} + #ifndef _WIN32 // Not run on Windows because this tests dlopen-specific behavior +// It also opens a plugin library outside class_loader, after which class_loader never unloads any +// library in this process, so tests that check unloading must come before it. // This is a different class name so that static variables in ClassLoader are isolated TEST(ClassLoaderGraveyardTest, loadUnloadLoadFromGraveyard) {