Repository: mesos Updated Branches: refs/heads/master 18c929b42 -> 941c32301
Allowed unloading of a single module. unloadAll() forces destruction of Owned pointers to the dynamic library handle. This implicitly causes a dlclose() of all the loaded module libraries. Unloading a dynamic library can have side effects if some shared state (e.g., a module) in Mesos points into the library. Futher more, removed the existing unloadAll() that is no longer needed. Review: https://reviews.apache.org/r/26789 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/941c3230 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/941c3230 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/941c3230 Branch: refs/heads/master Commit: 941c3230112876084ba5a6336942bc0dae945fd3 Parents: 18c929b Author: Kapil Arya <[email protected]> Authored: Thu Oct 16 13:16:31 2014 -0700 Committer: Niklas Q. Nielsen <[email protected]> Committed: Thu Oct 16 13:53:55 2014 -0700 ---------------------------------------------------------------------- src/module/manager.cpp | 42 ++++++++++++++++++++++------------------- src/module/manager.hpp | 14 +++++++------- src/tests/module_tests.cpp | 32 +++++++++++++++---------------- 3 files changed, 45 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/941c3230/src/module/manager.cpp ---------------------------------------------------------------------- diff --git a/src/module/manager.cpp b/src/module/manager.cpp index fdf0d33..2727fe0 100644 --- a/src/module/manager.cpp +++ b/src/module/manager.cpp @@ -41,7 +41,7 @@ namespace internal { pthread_mutex_t ModuleManager::mutex = PTHREAD_MUTEX_INITIALIZER; hashmap<const string, string> ModuleManager::kindToVersion; hashmap<const string, ModuleBase*> ModuleManager::moduleBases; -list<Owned<DynamicLibrary> > ModuleManager::dynamicLibraries; +hashmap<const string, Owned<DynamicLibrary>> ModuleManager::dynamicLibraries; void ModuleManager::initialize() @@ -83,13 +83,20 @@ void ModuleManager::initialize() } -// For testing only. Unload all dlopen()'d module libraries and -// clear the list of module manifests. -void ModuleManager::unloadAll() +// For testing only. Unload a given module and remove it from the list +// of ModuleBases. +Try<Nothing> ModuleManager::unload(const string& moduleName) { - kindToVersion.clear(); - moduleBases.clear(); - dynamicLibraries.clear(); + Lock lock(&mutex); + if (!moduleBases.contains(moduleName)) { + return Error( + "Error unloading module '" + moduleName + "': module not loaded"); + } + + // Do not remove the dynamiclibrary as it could result in unloading + // the library from the process memory. + moduleBases.erase(moduleName); + return Nothing(); } @@ -178,18 +185,15 @@ Try<Nothing> ModuleManager::load(const Modules& modules) return Error("Library name or path not provided"); } - Owned<DynamicLibrary> dynamicLibrary(new DynamicLibrary()); - Try<Nothing> result = dynamicLibrary->open(libraryName); - if (!result.isSome()) { - return Error("Error opening library: '" + libraryName + "'"); - } + if (!dynamicLibraries.contains(libraryName)) { + Owned<DynamicLibrary> dynamicLibrary(new DynamicLibrary()); + Try<Nothing> result = dynamicLibrary->open(libraryName); + if (!result.isSome()) { + return Error("Error opening library: '" + libraryName + "'"); + } - // Currently we never delete the DynamicLibrary instance nor do we - // expose a way to delete it so for now we just put it in a list. - // TODO(karya): If we add the functionality to "unload" a module - // library, we should make this pointer addressable by something - // like the module name. - dynamicLibraries.push_back(dynamicLibrary); + dynamicLibraries[libraryName] = dynamicLibrary; + } // Load module manifests. foreach (const string& moduleName, library.modules()) { @@ -202,7 +206,7 @@ Try<Nothing> ModuleManager::load(const Modules& modules) if (moduleBases.contains(moduleName)) { return Error("Error loading duplicate module '" + moduleName + "'"); } - Try<void*> symbol = dynamicLibrary->loadSymbol(moduleName); + Try<void*> symbol = dynamicLibraries[libraryName]->loadSymbol(moduleName); if (symbol.isError()) { return Error( "Error loading module '" + moduleName + "': " + symbol.error()); http://git-wip-us.apache.org/repos/asf/mesos/blob/941c3230/src/module/manager.hpp ---------------------------------------------------------------------- diff --git a/src/module/manager.hpp b/src/module/manager.hpp index ecf98ad..56e2af3 100644 --- a/src/module/manager.hpp +++ b/src/module/manager.hpp @@ -99,9 +99,9 @@ public: return "lib" + libraryName + libraryExtension; } - // Exposed just for testing so that we can close all open dynamic - // libraries. - static void unloadAll(); + // Exposed just for testing so that we can unload a given + // module and remove it from the list of ModuleBases. + static Try<Nothing> unload(const std::string& moduleName); private: static void initialize(); @@ -122,10 +122,10 @@ private: static hashmap<const std::string, ModuleBase*> moduleBases; // A list of dynamic libraries to keep the object from getting - // destructed. - // TODO(karya): Make it addressable only when we decide to implement - // something that lets remove the module library. - static std::list<process::Owned<DynamicLibrary> > dynamicLibraries; + // destructed. Destroying the DynamicLibrary object could result in + // unloading the library from the process memory. + static hashmap<const std::string, process::Owned<DynamicLibrary>> + dynamicLibraries; }; } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/941c3230/src/tests/module_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/module_tests.cpp b/src/tests/module_tests.cpp index 718fb7a..906f9ec 100644 --- a/src/tests/module_tests.cpp +++ b/src/tests/module_tests.cpp @@ -83,7 +83,7 @@ TEST_F(ModuleTest, ExampleModuleParseStringTest) EXPECT_EQ(module.get()->foo('A', 1024), 1089); EXPECT_EQ(module.get()->bar(0.5, 10.8), 5); - ModuleManager::unloadAll(); + EXPECT_SOME(ModuleManager::unload(moduleName)); } @@ -105,8 +105,6 @@ TEST_F(ModuleTest, AuthorInfoTest) EXPECT_EQ(stringify(moduleBase->authorName), "author"); EXPECT_EQ(stringify(moduleBase->authorEmail), "[email protected]"); EXPECT_EQ(stringify(moduleBase->description), "This is a test module."); - - ModuleManager::unloadAll(); } @@ -134,7 +132,7 @@ TEST_F(ModuleTest, LibraryNameWithoutExtension) // Reset LD_LIBRARY_PATH environment variable. os::setenv(ldLibraryPath, oldLdLibraryPath); - ModuleManager::unloadAll(); + EXPECT_SOME(ModuleManager::unload(moduleName)); } @@ -162,7 +160,7 @@ TEST_F(ModuleTest, LibraryNameWithExtension) // Reset LD_LIBRARY_PATH environment variable. os::setenv(ldLibraryPath, oldLdLibraryPath); - ModuleManager::unloadAll(); + EXPECT_SOME(ModuleManager::unload(moduleName)); } @@ -176,7 +174,7 @@ TEST_F(ModuleTest, EmptyLibraryFilename) EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -189,7 +187,7 @@ TEST_F(ModuleTest, EmptyModuleName) Modules modules = getModules(libraryName, moduleName); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -203,7 +201,7 @@ TEST_F(ModuleTest, UnknownLibraryTest) EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -217,7 +215,7 @@ TEST_F(ModuleTest, UnknownModuleTest) Modules modules = getModules(libraryName, moduleName); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -233,7 +231,7 @@ TEST_F(ModuleTest, UnknownModuleInstantiationTest) EXPECT_ERROR(ModuleManager::create<TestModule>("unknown")); - ModuleManager::unloadAll(); + EXPECT_SOME(ModuleManager::unload(moduleName)); } @@ -246,7 +244,7 @@ TEST_F(ModuleTest, NonModuleLibrary) Modules modules = getModules(libraryName, moduleName); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -265,7 +263,7 @@ TEST_F(ModuleTest, DuplicateModule) EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_SOME(ModuleManager::unload(moduleName)); } @@ -316,19 +314,19 @@ TEST_F(ModuleTest, DifferentApiVersion) updateModuleApiVersion(&library, moduleName, "0"); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); // Make the API version arbitrarily high. updateModuleApiVersion(&library, moduleName, "1000"); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); // Make the API version some random string. updateModuleApiVersion(&library, moduleName, "ThisIsNotAnAPIVersion!"); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -348,7 +346,7 @@ TEST_F(ModuleTest, NewerModuleLibrary) updateModuleLibraryVersion(&library, moduleName, "100.1.0"); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); } @@ -368,5 +366,5 @@ TEST_F(ModuleTest, OlderModuleLibrary) updateModuleLibraryVersion(&library, moduleName, "0.1.0"); EXPECT_ERROR(ModuleManager::load(modules)); - ModuleManager::unloadAll(); + EXPECT_ERROR(ModuleManager::unload(moduleName)); }
