Remove the ETS table from gen_mod

big_tests/tests/dynamic_modules.erl

@@ -22,14 +22,14 @@ get_saved_config(HostType, Module, Config) ->
 
 get_saved_config(#{node := NodeName}, HostType, Module, Config) ->
     SavedModules = proplists:get_value({saved_modules, NodeName, HostType}, Config),
-    proplists:get_value(Module, SavedModules).
+    maps:get(Module, SavedModules).
 
 ensure_modules(HostType, RequiredModules) ->
     ensure_modules(mim(), HostType, RequiredModules).
 
 ensure_modules(Node, HostType, RequiredModules) ->
     ToStop = [M || {M, stopped} <- RequiredModules],
-    ToEnsure = [{M, Opts} || {M, Opts} <- RequiredModules, Opts =/= stopped],
+    ToEnsure = maps:without(ToStop, maps:from_list(RequiredModules)),
     rpc(Node, mongoose_modules, replace_modules, [HostType, ToStop, ToEnsure]).
 
 ensure_stopped(HostType, ModulesToStop) ->
@@ -48,7 +48,7 @@ restore_modules(RPCSpec, Config) when is_map(RPCSpec) ->
 
 restore_modules(Node, HostType, SavedModules) ->
     CurrentModules = get_current_modules(Node, HostType),
-    ToStop = proplists:get_keys(CurrentModules) -- proplists:get_keys(SavedModules),
+    ToStop = maps:keys(CurrentModules) -- maps:keys(SavedModules),
     rpc(Node, mongoose_modules, replace_modules, [HostType, ToStop, SavedModules]).
 
 get_current_modules(HostType) ->

src/ejabberd_app.erl

@@ -52,7 +52,6 @@ start(normal, _Args) ->
     ejabberd_commands:init(),
     mongoose_commands:init(),
     mongoose_service:start(),
-    gen_mod:start(),
     mongoose_config:start(),
     mongoose_logs:set_global_loglevel(mongoose_config:get_opt(loglevel)),
     mongoose_deprecations:start(),

src/gen_mod.erl

@@ -44,8 +44,7 @@
               module_opts/0]).
 
 -export([
-         % Modules start & stop
-         start/0,
+         % Modules start & stop, do NOT use in the tests, use mongoose_modules API instead
          start_module/3,
          start_backend_module/2,
          start_backend_module/3,
@@ -80,11 +79,6 @@
 
 -include("mongoose.hrl").
 
--record(ejabberd_module, {
-          module_host_type, % {module(), host_type()},
-          opts % list()
-         }).
-
 -type module_feature() :: atom().
 -type domain_name() :: mongooseim:domain_name().
 -type host_type() :: mongooseim:host_type().
@@ -116,26 +110,17 @@
 
 -optional_callbacks([config_spec/0, supported_features/0, deps/2]).
 
--spec start() -> 'ok'.
-start() ->
-    ets:new(ejabberd_modules, [named_table, public, {read_concurrency, true},
-                               {keypos, #ejabberd_module.module_host_type}]),
-    ok.
-
-
+%% @doc This function should be called by mongoose_modules only.
+%% To start a new module at runtime, use mongoose_modules:ensure_module/3 instead.
 -spec start_module(HostType :: host_type(),
                    Module :: module(),
-                   Opts :: [any()]) -> {ok, term()} | {error, already_started}.
+                   Opts :: [any()]) -> {ok, term()}.
 start_module(HostType, Module, Opts) ->
-    case is_loaded(HostType, Module) of
-        true -> {error, already_started};
-        false -> start_module_for_host_type(HostType, Module, Opts)
-    end.
+    assert_loaded(HostType, Module),
+    start_module_for_host_type(HostType, Module, Opts).
 
 start_module_for_host_type(HostType, Module, Opts) ->
     {links, LinksBefore} = erlang:process_info(self(), links),
-    ets:insert(ejabberd_modules, #ejabberd_module{module_host_type = {Module, HostType},
-                                                  opts = Opts}),
     try
         lists:map(fun mongoose_service:assert_loaded/1,
                   get_required_services(HostType, Module, Opts)),
@@ -162,7 +147,6 @@ start_module_for_host_type(HostType, Module, Opts) ->
         end
     catch
         Class:Reason:StackTrace ->
-            ets:delete(ejabberd_modules, {Module, HostType}),
             ErrorText = io_lib:format("Problem starting the module ~p for "
                                       "host_type ~p~n options: ~p~n ~p: ~p~n~p",
                                       [Module, HostType, Opts, Class, Reason,
@@ -222,35 +206,27 @@ is_app_running(AppName) ->
     Timeout = 15000,
     lists:keymember(AppName, 1, application:which_applications(Timeout)).
 
-%% @doc Stop the module, and remove it from 'ejabberd_modules'
--spec stop_module(host_type(), module()) ->
-    {ok, list()} | {error, not_loaded} | {error, term()}.
+%% @doc This function should be called by mongoose_modules only.
+%% To stop a module at runtime, use mongoose_modules:ensure_stopped/2 instead.
+-spec stop_module(host_type(), module()) -> ok.
 stop_module(HostType, Module) ->
-    case is_loaded(HostType, Module) of
-        false -> {error, not_loaded};
-        true -> stop_module_for_host_type(HostType, Module)
-    end.
+    assert_loaded(HostType, Module),
+    stop_module_for_host_type(HostType, Module).
 
--spec stop_module_for_host_type(host_type(), module()) -> {error, term()} | {ok, list()}.
+-spec stop_module_for_host_type(host_type(), module()) -> ok.
 stop_module_for_host_type(HostType, Module) ->
-    Opts = get_module_opts(HostType, Module),
     try Module:stop(HostType) of
         {wait, ProcList} when is_list(ProcList) ->
-            lists:foreach(fun wait_for_process/1, ProcList),
-            ets:delete(ejabberd_modules, {Module, HostType}),
-            {ok, Opts};
+            lists:foreach(fun wait_for_process/1, ProcList);
         {wait, Process} ->
-            wait_for_process(Process),
-            ets:delete(ejabberd_modules, {Module, HostType}),
-            {ok, Opts};
+            wait_for_process(Process);
         _ ->
-            ets:delete(ejabberd_modules, {Module, HostType}),
-            {ok, Opts}
+            ok
     catch Class:Reason:Stacktrace ->
             ?LOG_ERROR(#{what => module_stopping_failed,
                          host_type => HostType, stop_module => Module,
                          class => Class, reason => Reason, stacktrace => Stacktrace}),
-            {error, Reason}
+            erlang:raise(Class, Reason, Stacktrace)
     end.
 
 -spec does_module_support(module(), module_feature()) -> boolean().
@@ -333,18 +309,10 @@ get_module_opt(HostType, Module, Opt, Default) ->
 
 
 get_module_opts(HostType, Module) ->
-    OptsList = ets:lookup(ejabberd_modules, {Module, HostType}),
-    case OptsList of
-        [] -> [];
-        [#ejabberd_module{opts = Opts} | _] -> Opts
-    end.
+    mongoose_config:get_opt([{modules, HostType}, Module], []).
 
 get_loaded_module_opts(HostType, Module) ->
-    OptsList = ets:lookup(ejabberd_modules, {Module, HostType}),
-    case OptsList of
-        [] -> error({module_not_loaded, HostType, Module});
-        [#ejabberd_module{opts = Opts} | _] -> Opts
-    end.
+    mongoose_config:get_opt([{modules, HostType}, Module]).
 
 -spec get_opt_subhost(domain_name(),
                       list(),
@@ -368,52 +336,33 @@ get_module_opt_subhost(Host, Module, Default) ->
 
 -spec loaded_modules() -> [module()].
 loaded_modules() ->
-    ModSet = ets:foldl(fun(#ejabberd_module{module_host_type = {Mod, _}}, Set) ->
-                               gb_sets:add_element(Mod, Set)
-                       end, gb_sets:new(), ejabberd_modules),
-    gb_sets:to_list(ModSet).
+    lists:usort(lists:flatmap(fun loaded_modules/1, ?ALL_HOST_TYPES)).
 
 -spec loaded_modules(host_type()) -> [module()].
 loaded_modules(HostType) ->
-    ets:select(ejabberd_modules,
-               [{#ejabberd_module{_ = '_', module_host_type = {'$1', HostType}},
-                 [],
-                 ['$1']}]).
+    maps:keys(mongoose_config:get_opt({modules, HostType})).
 
-
--spec loaded_modules_with_opts(host_type()) -> [{module(), list()}].
+-spec loaded_modules_with_opts(host_type()) -> #{module() => module_opts()}.
 loaded_modules_with_opts(HostType) ->
-    ets:select(ejabberd_modules,
-               [{#ejabberd_module{_ = '_', module_host_type = {'$1', HostType},
-                                  opts = '$2'},
-                 [],
-                 [{{'$1', '$2'}}]}]).
+    mongoose_config:get_opt({modules, HostType}).
 
--spec loaded_modules_with_opts() -> #{host_type() => [{module(), list()}]}.
+-spec loaded_modules_with_opts() -> #{host_type() => #{module() => module_opts()}}.
 loaded_modules_with_opts() ->
-    Res = ets:select(ejabberd_modules,
-               [{#ejabberd_module{_ = '_', module_host_type = {'$1', '$2'},
-                                  opts = '$3'},
-                 [],
-                 [{{'$2', '$1', '$3'}}]}]),
-    Hosts = lists:usort([H || {H, _, _} <- Res]),
-    maps:from_list([{H, [{M, Opts}
-                         || {HH, M, Opts} <- Res,
-                            H =:= HH]}
-                    || H <- Hosts]).
+    maps:from_list([{HostType, loaded_modules_with_opts(HostType)} || HostType <- ?ALL_HOST_TYPES]).
 
 -spec hosts_with_module(module()) -> [host_type()].
 hosts_with_module(Module) ->
-    ets:select(ejabberd_modules,
-               [{#ejabberd_module{_ = '_', module_host_type = {Module, '$1'}},
-                 [], ['$1']}]).
+    [HostType || HostType <- ?ALL_HOST_TYPES, is_loaded(HostType, Module)].
 
--spec hosts_and_opts_with_module(module()) -> [{host_type(), module_opts()}].
+-spec hosts_and_opts_with_module(module()) -> #{host_type() => module_opts()}.
 hosts_and_opts_with_module(Module) ->
-    ets:select(ejabberd_modules,
-               [{#ejabberd_module{_ = '_', module_host_type = {Module, '$1'},
-                                  opts = '$2'},
-                 [], [{{'$1', '$2'}}]}]).
+    maps:from_list(
+      lists:flatmap(fun(HostType) ->
+                            case mongoose_config:lookup_opt([{modules, HostType}, Module]) of
+                                {error, not_found} -> [];
+                                {ok, Opts} -> [{HostType, Opts}]
+                            end
+                    end, ?ALL_HOST_TYPES)).
 
 -spec get_module_proc(binary() | string(), module()) -> atom().
 %% TODO:
@@ -425,10 +374,21 @@ get_module_proc(Host, Base) when is_binary(Host) ->
 get_module_proc(Host, Base) ->
     list_to_atom(atom_to_list(Base) ++ "_" ++ Host).
 
+-spec assert_loaded(mongooseim:host_type(), module()) -> ok.
+assert_loaded(HostType, Module) ->
+    case is_loaded(HostType, Module) of
+        true ->
+            ok;
+        false ->
+            error(#{what => module_not_loaded,
+                    text => <<"Module missing from mongoose_config">>,
+                    host_type => HostType,
+                    module => Module})
+    end.
 
 -spec is_loaded(HostType :: binary(), Module :: atom()) -> boolean().
 is_loaded(HostType, Module) ->
-    ets:member(ejabberd_modules, {Module, HostType}).
+    maps:is_key(Module, loaded_modules_with_opts(HostType)).
 
 -spec get_deps(HostType :: host_type(), Module :: module(),
                Opts :: proplists:proplist()) -> module_deps_list().

src/mongoose_modules.erl

@@ -32,14 +32,14 @@ stop() ->
 %% Running modules from ToStop are stopped and modules from ToEnsure are (re)started when needed.
 %% Unused dependencies are stopped if no running modules depend on them anymore.
 %% To prevent an unused dependency from being stopped, you need to include it in ToEnsure.
--spec replace_modules(mongooseim:host_type(), [module()], module_list()) -> ok.
+-spec replace_modules(mongooseim:host_type(), [module()], module_map()) -> ok.
 replace_modules(HostType, ToStop, ToEnsure) ->
     Current = get_modules(HostType),
-    Old = maps:with(ToStop ++ proplists:get_keys(ToEnsure), Current),
+    Old = maps:with(ToStop ++ maps:keys(ToEnsure), Current),
     OldWithDeps = gen_mod_deps:resolve_deps(HostType, Old),
     SortedOldWithDeps = gen_mod_deps:sort_deps(HostType, OldWithDeps),
     WithoutOld = maps:without(maps:keys(OldWithDeps), Current),
-    WithNew = maps:merge(WithoutOld, maps:from_list(ToEnsure)),
+    WithNew = maps:merge(WithoutOld, ToEnsure),
     Target = gen_mod_deps:resolve_deps(HostType, WithNew),
 
     %% Stop each affected module if it is not in Target (stop deps first)
@@ -88,7 +88,7 @@ ensure_started(HostType, Module, RawOpts) ->
 start_module(HostType, Module, Opts, Modules) ->
     set_modules(HostType, Modules#{Module => Opts}),
     try
-        {ok, _} = gen_mod:start_module(HostType, Module, Opts)
+        gen_mod:start_module(HostType, Module, Opts)
     catch
         C:R:S ->
             set_modules(HostType, Modules),
@@ -97,7 +97,7 @@ start_module(HostType, Module, Opts, Modules) ->
 
 -spec stop_module(mongooseim:host_type(), module(), module_map()) -> ok.
 stop_module(HostType, Module, Modules) ->
-    {ok, _} = gen_mod:stop_module(HostType, Module),
+    gen_mod:stop_module(HostType, Module),
     set_modules(HostType, maps:remove(Module, Modules)).
 
 -spec sorted_modules(mongooseim:host_type()) -> module_list().

test/auth_tokens_SUITE.erl

@@ -52,25 +52,24 @@ init_per_testcase(Test, Config)
              Test =:= validation_property;
              Test =:= choose_key_by_token_type ->
     mock_mongoose_metrics(),
-    Config1 = async_helper:start(Config, [{gen_hook, start_link, []},
-                                          {gen_mod, start, []}]),
+    Config1 = async_helper:start(Config, [{gen_hook, start_link, []}]),
     mock_keystore(),
     mock_rdbms_backend(),
     Config1;
 
 init_per_testcase(validity_period_test, Config) ->
+    mongoose_config:set_opt({modules, host_type()},
+                            #{?TESTED => validity_period_cfg(access, {13, hours})}),
     mock_rdbms_backend(),
     mock_mongoose_metrics(),
     mock_gen_iq_handler(),
     mock_ejabberd_commands(),
-    async_helper:start(Config, [{gen_mod, start, []},
-                                {gen_hook, start_link, []}]);
+    async_helper:start(Config, [{gen_hook, start_link, []}]);
 
 init_per_testcase(revoked_token_is_not_valid, Config) ->
     mock_mongoose_metrics(),
     mock_tested_backend(),
-    Config1 = async_helper:start(Config, [{gen_mod, start, []},
-                                          {gen_hook, start_link, []}]),
+    Config1 = async_helper:start(Config, [{gen_hook, start_link, []}]),
     mock_keystore(),
     Config1;
 
@@ -87,6 +86,7 @@ end_per_testcase(Test, C)
     C;
 
 end_per_testcase(validity_period_test, C) ->
+    mongoose_config:unset_opt({modules, host_type()}),
     meck:unload(mod_auth_token_backend),
     meck:unload(mongoose_metrics),
     meck:unload(gen_iq_handler),
@@ -139,8 +139,7 @@ validation_property(_) ->
 
 validity_period_test(_) ->
     %% given
-    ok = ?TESTED:start(host_type(),
-                       validity_period_cfg(access, {13, hours})),
+    ok = ?TESTED:start(host_type(), mongoose_config:get_opt([{modules, host_type()}, ?TESTED])),
     UTCSeconds = utc_now_as_seconds(),
     ExpectedSeconds = UTCSeconds + (    13 %% hours
                                     * 3600 %% seconds per hour
@@ -223,9 +222,7 @@ utc_now_as_seconds() ->
 %%                              {{validity_period, refresh}, {13, days}}]}
 %%           ]}.
 validity_period_cfg(Type, Period) ->
-    Opts = [ {{validity_period, Type}, Period} ],
-    ets:insert(ejabberd_modules, {ejabberd_module, {?TESTED, host_type()}, Opts}),
-    Opts.
+    [{{validity_period, Type}, Period}].
 
 %% This is a negative test case helper - that's why we invert the logic below.
 %% I.e. we expect the property to fail.