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) {