Unbind destroyed ClassLoaders from their library, fix ownership data races, add stress tests

include/class_loader/class_loader.hpp

@@ -275,7 +275,12 @@ class ClassLoader
              if (context->loader) {
                context->loader->onPluginDeletion<Base>(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<rcpputils::SharedLibrary> library;
+    std::size_t orphaned_instances = 0;
   };
   std::shared_ptr<DeleterContext> deleter_context_;
 };

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>(args)...);
-  } else if (factory && factory->isOwnedBy(nullptr)) {
+  } else if (owned_by_nobody) {
     CONSOLE_BRIDGE_logDebug(
       "%s",
       "class_loader.impl: ALERT!!! "

src/class_loader.cpp

@@ -75,17 +75,41 @@ ClassLoader::~ClassLoader()
     "class_loader.ClassLoader: Destroying class loader, "
     "unloading associated library...\n");
   std::lock_guard<std::recursive_mutex> lock(deleter_context_->mutex);
-  {
-    // The deferred on-demand unload in onPluginDeletion() can't happen once this loader is gone.
-    std::lock_guard<std::recursive_mutex> 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<std::recursive_mutex> ref_count_lock(plugin_ref_count_mutex_);
+  std::lock_guard<std::recursive_mutex> 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<rcpputils::SharedLibrary>(getLibraryPath());
+      deleter_context_->orphaned_instances = static_cast<std::size_t>(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;
 }
 

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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> llv_lock(getLoadedLibraryVectorMutex());
+  std::lock_guard<std::recursive_mutex> 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 =

test/utest.cpp

@@ -33,7 +33,10 @@
 #include <dlfcn.h>
 #endif
 
+#include <algorithm>
+#include <atomic>
 #include <memory>
+#include <new>
 #include <string>
 #include <thread>
 #include <tuple>
@@ -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<class_loader::ClassLoader>(LIBRARY_2, on_demand);
+    std::shared_ptr<Base> shared = loader->createInstance<Base>("Robot");
+    class_loader::ClassLoader::UniquePtr<Base> unique = loader->createUniqueInstance<Base>("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<class_loader::ClassLoader>(LIBRARY_2, false);
+  std::shared_ptr<Base> orphan = loader->createInstance<Base>("Robot");
+  loader.reset();
+
+  class_loader::ClassLoader loader2(LIBRARY_2, false);
+  EXPECT_EQ("Beep boop", loader2.createInstance<Base>("Robot")->saySomething());
+  orphan.reset();
+  EXPECT_TRUE(loader2.isLibraryLoaded());
+  EXPECT_EQ("Beep boop", loader2.createUniqueInstance<Base>("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<int>(c));
+    alignas(class_loader::ClassLoader) unsigned char storage[sizeof(class_loader::ClassLoader)];
+    std::shared_ptr<Base> dog;
+    {
+      auto * loader = new (storage) class_loader::ClassLoader(
+        LIBRARY_1, c == Case::kLiveInstanceOnDemand);
+      if (c == Case::kLoadedTwice) {
+        loader->loadLibrary();
+      } else {
+        dog = loader->createInstance<Base>("Dog");
+      }
+      loader->~ClassLoader();
+    }
+    {
+      auto * other = new (storage) class_loader::ClassLoader(LIBRARY_2, true);
+      EXPECT_FALSE(other->isClassAvailable<Base>("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<std::thread> 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<Base>("Dog")->saySomething());
+          EXPECT_EQ("Meow", loader.createUniqueInstance<Base>("Cat")->saySomething());
+        }
+      });
+  }
+  for (auto & thread : threads) {
+    thread.join();
+  }
+  EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1));
+}
+
+TEST(ClassLoaderStressTest, multiLibraryClassLoadersOnManyThreads) {
+  std::vector<std::thread> 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<Base>();
+            EXPECT_NE(classes.end(), std::ranges::find(classes, "Dog"));
+            EXPECT_NE(classes.end(), std::ranges::find(classes, "Robot"));
+          }
+          EXPECT_EQ("Bark", loader.createInstance<Base>("Dog")->saySomething());
+          EXPECT_EQ("Beep boop", loader.createUniqueInstance<Base>("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<class_loader::ClassLoader>(LIBRARY_1, i % 2 == 0);
+    std::vector<std::shared_ptr<Base>> instances;
+    for (size_t t = 0; t < STRESS_THREADS; ++t) {
+      instances.push_back(loader->createInstance<Base>("Dog"));
+    }
+    std::atomic<bool> start{false};
+    std::vector<std::thread> 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) {