Update ModuleManager to use synchronized. Review: https://reviews.apache.org/r/35098
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/98039c99 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/98039c99 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/98039c99 Branch: refs/heads/master Commit: 98039c99b0a2e0a36a7d4c22e672ecd817923bb4 Parents: 8939609 Author: Joris Van Remoortere <[email protected]> Authored: Sat Jun 13 07:15:59 2015 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Sun Jun 14 02:43:01 2015 -0700 ---------------------------------------------------------------------- src/module/manager.cpp | 129 ++++++++++++++++++++++---------------------- src/module/manager.hpp | 71 ++++++++++++------------ 2 files changed, 101 insertions(+), 99 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/98039c99/src/module/manager.cpp ---------------------------------------------------------------------- diff --git a/src/module/manager.cpp b/src/module/manager.cpp index 2c7f876..909ca56 100644 --- a/src/module/manager.cpp +++ b/src/module/manager.cpp @@ -16,6 +16,7 @@ * limitations under the License. */ +#include <mutex> #include <string> #include <vector> @@ -31,8 +32,6 @@ #include <stout/stringify.hpp> #include <stout/version.hpp> -#include "common/lock.hpp" - #include "messages/messages.hpp" #include "module/manager.hpp" @@ -46,7 +45,7 @@ using namespace mesos; using namespace mesos::internal; using namespace mesos::modules; -pthread_mutex_t ModuleManager::mutex = PTHREAD_MUTEX_INITIALIZER; +std::mutex ModuleManager::mutex; hashmap<const string, string> ModuleManager::kindToVersion; hashmap<const string, ModuleBase*> ModuleManager::moduleBases; hashmap<const string, Owned<DynamicLibrary>> ModuleManager::dynamicLibraries; @@ -104,15 +103,16 @@ void ModuleManager::initialize() // of ModuleBases. Try<Nothing> ModuleManager::unload(const string& moduleName) { - Lock lock(&mutex); - if (!moduleBases.contains(moduleName)) { - return Error( - "Error unloading module '" + moduleName + "': module not loaded"); - } + synchronized (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); + // Do not remove the dynamiclibrary as it could result in + // unloading the library from the process memory. + moduleBases.erase(moduleName); + } return Nothing(); } @@ -189,64 +189,67 @@ Try<Nothing> ModuleManager::verifyModule( Try<Nothing> ModuleManager::load(const Modules& modules) { - Lock lock(&mutex); - initialize(); - - foreach (const Modules::Library& library, modules.libraries()) { - string libraryName; - if (library.has_file()) { - libraryName = library.file(); - } else if (library.has_name()) { - libraryName = os::libraries::expandName(library.name()); - } else { - return Error("Library name or path not provided"); - } - - if (!dynamicLibraries.contains(libraryName)) { - Owned<DynamicLibrary> dynamicLibrary(new DynamicLibrary()); - Try<Nothing> result = dynamicLibrary->open(libraryName); - if (!result.isSome()) { - return Error( - "Error opening library: '" + libraryName + "': " + result.error()); + synchronized (mutex) { + initialize(); + + foreach (const Modules::Library& library, modules.libraries()) { + string libraryName; + if (library.has_file()) { + libraryName = library.file(); + } else if (library.has_name()) { + libraryName = os::libraries::expandName(library.name()); + } else { + return Error("Library name or path not provided"); } - dynamicLibraries[libraryName] = dynamicLibrary; - } - - // Load module manifests. - foreach (const Modules::Library::Module& module, library.modules()) { - if (!module.has_name()) { - return Error( - "Error: module name not provided with 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 + + "': " + result.error()); + } - // Check for possible duplicate module names. - const std::string moduleName = module.name(); - if (moduleBases.contains(moduleName)) { - return Error("Error loading duplicate module '" + moduleName + "'"); + dynamicLibraries[libraryName] = dynamicLibrary; } - // Load ModuleBase. - Try<void*> symbol = dynamicLibraries[libraryName]->loadSymbol(moduleName); - if (symbol.isError()) { - return Error( - "Error loading module '" + moduleName + "': " + symbol.error()); + // Load module manifests. + foreach (const Modules::Library::Module& module, library.modules()) { + if (!module.has_name()) { + return Error( + "Error: module name not provided with library '" + libraryName + + "'"); + } + + // Check for possible duplicate module names. + const std::string moduleName = module.name(); + if (moduleBases.contains(moduleName)) { + return Error("Error loading duplicate module '" + moduleName + "'"); + } + + // Load ModuleBase. + Try<void*> symbol = + dynamicLibraries[libraryName]->loadSymbol(moduleName); + if (symbol.isError()) { + return Error( + "Error loading module '" + moduleName + "': " + symbol.error()); + } + ModuleBase* moduleBase = (ModuleBase*) symbol.get(); + + // Verify module compatibility including version, etc. + Try<Nothing> result = verifyModule(moduleName, moduleBase); + if (result.isError()) { + return Error( + "Error verifying module '" + moduleName + "': " + result.error()); + } + + moduleBases[moduleName] = (ModuleBase*) symbol.get(); + + // Now copy the supplied module-specific parameters. + moduleParameters[moduleName].mutable_parameter()->CopyFrom( + module.parameters()); } - ModuleBase* moduleBase = (ModuleBase*) symbol.get(); - - // Verify module compatibility including version, etc. - Try<Nothing> result = verifyModule(moduleName, moduleBase); - if (result.isError()) { - return Error( - "Error verifying module '" + moduleName + "': " + result.error()); - } - - moduleBases[moduleName] = (ModuleBase*) symbol.get(); - - // Now copy the supplied module-specific parameters. - moduleParameters[moduleName].mutable_parameter()->CopyFrom( - module.parameters()); } } http://git-wip-us.apache.org/repos/asf/mesos/blob/98039c99/src/module/manager.hpp ---------------------------------------------------------------------- diff --git a/src/module/manager.hpp b/src/module/manager.hpp index 4befb64..cab67a8 100644 --- a/src/module/manager.hpp +++ b/src/module/manager.hpp @@ -19,9 +19,8 @@ #ifndef __MODULE_MANAGER_HPP__ #define __MODULE_MANAGER_HPP__ -#include <pthread.h> - #include <list> +#include <mutex> #include <string> #include <vector> @@ -37,8 +36,8 @@ #include <stout/check.hpp> #include <stout/dynamiclibrary.hpp> #include <stout/hashmap.hpp> +#include <stout/synchronized.hpp> -#include "common/lock.hpp" #include "messages/messages.hpp" namespace mesos { @@ -71,40 +70,42 @@ public: template <typename T> static Try<T*> create(const std::string& moduleName) { - mesos::internal::Lock lock(&mutex); - if (!moduleBases.contains(moduleName)) { - return Error( - "Module '" + moduleName + "' unknown"); - } + synchronized (mutex) { + if (!moduleBases.contains(moduleName)) { + return Error( + "Module '" + moduleName + "' unknown"); + } - Module<T>* module = (Module<T>*) moduleBases[moduleName]; - if (module->create == NULL) { - return Error( - "Error creating module instance for '" + moduleName + "': " - "create() method not found"); - } + Module<T>* module = (Module<T>*) moduleBases[moduleName]; + if (module->create == NULL) { + return Error( + "Error creating module instance for '" + moduleName + "': " + "create() method not found"); + } - std::string expectedKind = kind<T>(); - if (expectedKind != module->kind) { - return Error( - "Error creating module instance for '" + moduleName + "': " - "module is of kind '" + module->kind + "', but the requested " - "kind is '" + expectedKind + "'"); - } + std::string expectedKind = kind<T>(); + if (expectedKind != module->kind) { + return Error( + "Error creating module instance for '" + moduleName + "': " + "module is of kind '" + module->kind + "', but the requested " + "kind is '" + expectedKind + "'"); + } - T* instance = module->create(moduleParameters[moduleName]); - if (instance == NULL) { - return Error("Error creating Module instance for '" + moduleName + "'"); + T* instance = module->create(moduleParameters[moduleName]); + if (instance == NULL) { + return Error("Error creating Module instance for '" + moduleName + "'"); + } + return instance; } - return instance; } template <typename T> static bool contains(const std::string& moduleName) { - mesos::internal::Lock lock(&mutex); - return (moduleBases.contains(moduleName) && - moduleBases[moduleName]->kind == stringify(kind<T>())); + synchronized (mutex) { + return (moduleBases.contains(moduleName) && + moduleBases[moduleName]->kind == stringify(kind<T>())); + } } // Returns all module names that have been loaded that implement the @@ -117,13 +118,13 @@ public: template <typename T> static std::vector<std::string> find() { - mesos::internal::Lock lock(&mutex); - std::vector<std::string> names; - foreachpair (const std::string& name, ModuleBase* base, moduleBases) { - if (base->kind == stringify(kind<T>())) { - names.push_back(name); + synchronized (mutex) { + foreachpair (const std::string& name, ModuleBase* base, moduleBases) { + if (base->kind == stringify(kind<T>())) { + names.push_back(name); + } } } @@ -141,9 +142,7 @@ private: const std::string& moduleName, const ModuleBase* moduleBase); - // TODO(karya): Replace pthread_mutex_t with std::mutex in - // common/lock.hpp and other places that refer to it. - static pthread_mutex_t mutex; + static std::mutex mutex; static hashmap<const std::string, std::string> kindToVersion;
