From c8db7f88dc48925a8e2ef06010f4dc72ca58f6d9 Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Thu, 5 Dec 2019 12:37:04 +0100 Subject: [PATCH] Move mg.reload procedures to the builtin module Reviewers: mferencevic, ipaljak Reviewed By: mferencevic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2584 --- src/query/plan/operator.cpp | 43 +++---------------------- src/query/procedure/module.cpp | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 39 deletions(-) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 7f5aca86d..50f066057 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -3735,39 +3735,6 @@ std::vector CallProcedure::ModifiedSymbols( namespace { -// Return true if we handled one of the special `mg` module procedures for -// reloading query modules. -// @throw QueryRuntimeException in case of error during procedure invocation. -bool HandleReloadProcedures( - const std::string_view &fully_qualified_procedure_name, - const std::vector &args, ExpressionEvaluator *evaluator) { - // It would be great to simply register `reload_all_modules` as a - // regular procedure on a `mg` module, so we don't have a special case here. - // Unfortunately, reloading requires taking a write lock, and we would - // acquire a read lock by getting the module. - if (fully_qualified_procedure_name == "mg.reload_all_modules") { - if (!args.empty()) - throw QueryRuntimeException( - "'mg.reload_all_modules' requires no arguments."); - procedure::gModuleRegistry.ReloadAllModules(); - return true; - } else if (fully_qualified_procedure_name == "mg.reload") { - // This is a special case for the same reasons as `mg.reload_all_modules`. - if (args.size() != 1U) - throw QueryRuntimeException("'mg.reload' requires exactly 1 argument."); - const auto &arg = args.front()->Accept(*evaluator); - if (!arg.IsString()) { - throw QueryRuntimeException( - "'mg.reload' argument named 'module_name' at position 0 must be of " - "type STRING."); - } - const auto &module_name = arg.ValueString(); - procedure::gModuleRegistry.ReloadModuleNamed(module_name); - return true; - } - return false; -} - // Return the ModulePtr and `mgp_proc *` of the found procedure after resolving // `fully_qualified_procedure_name`. `memory` is used for temporary allocations // inside this function. ModulePtr must be kept alive to make sure it won't be @@ -3906,15 +3873,13 @@ class CallProcedureCursor : public Cursor { ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, context.db_accessor, graph_view); - // First try to handle special procedures for (re)loading modules. - if (HandleReloadProcedures(self_->procedure_name_, self_->arguments_, - &evaluator)) - continue; - // Nothing special, so find the regular procedure and invoke it. // It might be a good idea to resolve the procedure name once, at the // start. Unfortunately, this could deadlock if we tried to invoke a // procedure from a module (read lock) and reload a module (write lock) - // inside the same execution thread. + // inside the same execution thread. Also, our RWLock is setup so that + // it's not possible for a single thread to request multiple read locks. + // Builtin module registration in query/procedure/module.cpp depends on + // this locking scheme. const auto &[module, proc] = FindProcedureOrThrow( self_->procedure_name_, context.evaluation_context.memory); result_.signature = &proc->results; diff --git a/src/query/procedure/module.cpp b/src/query/procedure/module.cpp index a1334d6c3..12ad1b09f 100644 --- a/src/query/procedure/module.cpp +++ b/src/query/procedure/module.cpp @@ -74,8 +74,64 @@ bool CloseModule(Module *module) { return true; } +// Return true if the module is builtin, i.e. not loaded from dynamic lib. +// Builtin modules cannot be reloaded nor unloaded. bool IsBuiltinModule(const Module &module) { return module.handle == nullptr; } +void RegisterMgReload(ModuleRegistry *module_registry, utils::RWLock *lock, + Module *module) { + // Reloading relies on the fact that regular procedure invocation through + // CallProcedureCursor::Pull takes ModuleRegistry::lock_ with READ access. To + // reload modules we have to upgrade our READ access to WRITE access, + // therefore we release the READ lock and invoke the reload function which + // takes the WRITE lock. Obviously, some other thread may take a READ or WRITE + // lock during our transition when we hold no such lock. In this case it is + // fine, because our builtin module cannot be unloaded and we are ok with + // using the new state of module_registry when we manage to acquire the lock + // we desire. Note, deadlock between threads should not be possible, because a + // single thread may only take either a READ or a WRITE lock, it's not + // possible for a thread to hold both. If a thread tries to do that, it will + // deadlock immediately (no other thread needs to do anything). + auto with_unlock_shared = [lock](const auto &reload_function) { + lock->unlock_shared(); + try { + reload_function(); + // There's no finally in C++, but we have to return our original READ lock + // state in any possible case. + } catch (...) { + lock->lock_shared(); + throw; + } + lock->lock_shared(); + }; + auto reload_all_cb = [module_registry, with_unlock_shared]( + const mgp_list *, const mgp_graph *, mgp_result *res, + mgp_memory *) { + bool succ = false; + with_unlock_shared([&]() { succ = module_registry->ReloadAllModules(); }); + if (!succ) mgp_result_set_error_msg(res, "Failed to reload all modules."); + }; + mgp_proc reload_all("reload_all", reload_all_cb, utils::NewDeleteResource()); + module->procedures.emplace("reload_all", std::move(reload_all)); + auto reload_cb = [module_registry, with_unlock_shared]( + const mgp_list *args, const mgp_graph *, mgp_result *res, + mgp_memory *) { + CHECK(mgp_list_size(args) == 1U) << "Should have been type checked already"; + const mgp_value *arg = mgp_list_at(args, 0); + CHECK(mgp_value_is_string(arg)) << "Should have been type checked already"; + bool succ = false; + with_unlock_shared([&]() { + succ = module_registry->ReloadModuleNamed(mgp_value_get_string(arg)); + }); + if (!succ) + mgp_result_set_error_msg( + res, "Failed to reload the module; it is no longer loaded."); + }; + mgp_proc reload("reload", reload_cb, utils::NewDeleteResource()); + mgp_proc_add_arg(&reload, "module_name", mgp_type_string()); + module->procedures.emplace("reload", std::move(reload)); +} + void RegisterMgProcedures( // We expect modules to be sorted by name. const std::map> *all_modules, @@ -140,6 +196,7 @@ void RegisterMgProcedures( ModuleRegistry::ModuleRegistry() { Module module{.handle = nullptr}; RegisterMgProcedures(&modules_, &module); + RegisterMgReload(this, &lock_, &module); modules_.emplace("mg", std::move(module)); }