Merge pull request #3878 from esl/hooks/persistent_term

Put hooks into persistent_term using batching

src/ejabberd_app.erl

@@ -80,6 +80,7 @@ do_start() ->
     mongoose_listener:start(),
     ejabberd_admin:start(),
     mongoose_metrics:init_mongooseim_metrics(),
+    gen_hook:reload_hooks(),
     update_status_file(started),
     ?LOG_NOTICE(#{what => mongooseim_node_started, version => ?MONGOOSE_VERSION, node => node()}),
     Sup.

src/gen_hook.erl

@@ -9,6 +9,7 @@
          add_handlers/1,
          delete_handlers/1,
          run_fold/4]).
+-export([reload_hooks/0]).
 
 %% gen_server callbacks
 -export([init/1,
@@ -70,7 +71,6 @@
                        hook_fn :: hook_fn(),
                        extra :: extra()}).
 
--define(TABLE, ?MODULE).
 %%%----------------------------------------------------------------------
 %%% API
 %%%----------------------------------------------------------------------
@@ -135,59 +135,66 @@ delete_handler({HookName, Tag, _, _, _} = HookTuple) ->
                Params :: hook_params()) -> hook_fn_ret().
 run_fold(HookName, Tag, Acc, Params) ->
     Key = hook_key(HookName, Tag),
-    case ets:lookup(?TABLE, Key) of
-        [{_, Ls}] ->
+    case persistent_term:get(?MODULE, #{}) of
+        #{Key := Ls} ->
             mongoose_metrics:increment_generic_hook_metric(Tag, HookName),
             run_hook(Ls, Acc, Params, Key);
-        [] ->
+        _ ->
             {ok, Acc}
     end.
 
+reload_hooks() ->
+    gen_server:call(?MODULE, reload_hooks).
+
 %%%----------------------------------------------------------------------
 %%% gen_server callback functions
 %%%----------------------------------------------------------------------
 
 init([]) ->
-    ets:new(?TABLE, [named_table, {read_concurrency, true}]),
-    {ok, no_state}.
+    erlang:process_flag(trap_exit, true), %% We need to make sure that terminate is called in tests
+    {ok, #{}}.
 
 handle_call({add_handler, Key, #hook_handler{} = HookHandler}, _From, State) ->
-    Reply = case ets:lookup(?TABLE, Key) of
-                [{_, Ls}] ->
-                    case lists:search(fun_is_handler_equal_to(HookHandler), Ls) of
-                        {value, _} ->
-                            ?LOG_WARNING(#{what => duplicated_handler,
-                                           key => Key, handler => HookHandler}),
-                            ok;
-                        false ->
-                            %% NOTE: sort *only* on the priority,
-                            %% order of other fields is not part of the contract
-                            NewLs = lists:keymerge(#hook_handler.prio, Ls, [HookHandler]),
-                            ets:insert(?TABLE, {Key, NewLs}),
-                            ok
-                    end;
-                [] ->
-                    NewLs = [HookHandler],
-                    ets:insert(?TABLE, {Key, NewLs}),
-                    create_hook_metric(Key),
-                    ok
-            end,
-    {reply, Reply, State};
+    NewState =
+        case maps:get(Key, State, []) of
+            [] ->
+                NewLs = [HookHandler],
+                create_hook_metric(Key),
+                maps:put(Key, NewLs, State);
+            Ls ->
+                case lists:search(fun_is_handler_equal_to(HookHandler), Ls) of
+                    {value, _} ->
+                        ?LOG_WARNING(#{what => duplicated_handler,
+                                       key => Key, handler => HookHandler}),
+                        State;
+                    false ->
+                        %% NOTE: sort *only* on the priority,
+                        %% order of other fields is not part of the contract
+                        NewLs = lists:keymerge(#hook_handler.prio, Ls, [HookHandler]),
+                        maps:put(Key, NewLs, State)
+                end
+        end,
+    maybe_insert_immediately(NewState),
+    {reply, ok, NewState};
 handle_call({delete_handler, Key, #hook_handler{} = HookHandler}, _From, State) ->
-    Reply = case ets:lookup(?TABLE, Key) of
-                [{_, Ls}] ->
-                    %% NOTE: The straightforward handlers comparison would compare
-                    %% the function objects, which is not well-defined in OTP.
-                    %% So we do a manual comparison on the MFA of the funs,
-                    %% by using `erlang:fun_info/2`
-                    Pred = fun_is_handler_equal_to(HookHandler),
-                    {_, NewLs} = lists:partition(Pred, Ls),
-                    ets:insert(?TABLE, {Key, NewLs}),
-                    ok;
-                [] ->
-                    ok
-            end,
-    {reply, Reply, State};
+    NewState =
+        case maps:get(Key, State, []) of
+            [] ->
+                State;
+            Ls ->
+                %% NOTE: The straightforward handlers comparison would compare
+                %% the function objects, which is not well-defined in OTP.
+                %% So we do a manual comparison on the MFA of the funs,
+                %% by using `erlang:fun_info/2`
+                Pred = fun_is_handler_equal_to(HookHandler),
+                {_, NewLs} = lists:partition(Pred, Ls),
+                maps:put(Key, NewLs, State)
+        end,
+    maybe_insert_immediately(NewState),
+    {reply, ok, NewState};
+handle_call(reload_hooks, _From, State) ->
+    persistent_term:put(?MODULE, State),
+    {reply, ok, State};
 handle_call(Request, From, State) ->
     ?UNEXPECTED_CALL(Request, From),
     {reply, bad_request, State}.
@@ -201,7 +208,7 @@ handle_info(Info, State) ->
     {noreply, State}.
 
 terminate(_Reason, _State) ->
-    ets:delete(?TABLE),
+    persistent_term:erase(?MODULE),
     ok.
 
 code_change(_OldVsn, State, _Extra) ->
@@ -210,6 +217,19 @@ code_change(_OldVsn, State, _Extra) ->
 %%%----------------------------------------------------------------------
 %%% Internal functions
 %%%----------------------------------------------------------------------
+
+%% @doc This call inserts the new hooks map immediately only if an existing map was already present.
+%% This simplifies tests: at startup we wait until all hooks have been accumulated
+%% before inserting them all at once, while during tests we don't need to remember
+%% to reload on every change
+maybe_insert_immediately(State) ->
+    case persistent_term:get(?MODULE, hooks_not_set) of
+        hooks_not_set ->
+            ok;
+        _ ->
+            persistent_term:put(?MODULE, State)
+    end.
+
 -spec run_hook([#hook_handler{}], hook_acc(), hook_params(), key()) -> hook_fn_ret().
 run_hook([], Acc, _Params, _Key) ->
     {ok, Acc};

src/metrics/mongoose_metrics.erl

@@ -145,7 +145,7 @@ get_global_metric_names() ->
 get_aggregated_values(Metric) ->
     exometer:aggregate([{{['_', Metric], '_', '_'}, [], [true]}], [one, count, value]).
 
--spec increment_generic_hook_metric(mongooseim:host_type(), atom()) -> ok | {error, any()}.
+-spec increment_generic_hook_metric(mongooseim:host_type_or_global(), atom()) -> ok | {error, any()}.
 increment_generic_hook_metric(HostType, Hook) ->
     UseOrSkip = filter_hook(Hook),
     do_increment_generic_hook_metric(HostType, Hook, UseOrSkip).

test/auth_tokens_SUITE.erl

@@ -56,7 +56,7 @@ 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, []}]),
+    Config1 = async_helper:start(Config, [{mongooseim_helper, start_link_loaded_hooks, []}]),
     mock_keystore(),
     mock_rdbms_backend(),
     Config1;
@@ -66,12 +66,12 @@ init_per_testcase(validity_period_test, Config) ->
     mock_mongoose_metrics(),
     mock_gen_iq_handler(),
     mock_ejabberd_commands(),
-    async_helper:start(Config, [{gen_hook, start_link, []}]);
+    async_helper:start(Config, [{mongooseim_helper, start_link_loaded_hooks, []}]);
 
 init_per_testcase(revoked_token_is_not_valid, Config) ->
     mock_mongoose_metrics(),
     mock_tested_backend(),
-    Config1 = async_helper:start(Config, [{gen_hook, start_link, []}]),
+    Config1 = async_helper:start(Config, [{mongooseim_helper, start_link_loaded_hooks, []}]),
     mock_keystore(),
     Config1;
 

test/component_reg_SUITE.erl

@@ -18,13 +18,13 @@ init_per_suite(C) ->
     meck:expect(mongoose_domain_api, get_host_type,
                 fun(_) -> {error, not_found} end),
     application:ensure_all_started(exometer_core),
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     ejabberd_router:start_link(),
     C.
 
 init_per_testcase(_, C) ->
     mongoose_router:start(),
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     C.
 
 end_per_suite(_C) ->
@@ -48,7 +48,7 @@ registering(_C) ->
     ok.
 
 registering_with_local(_C) ->
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     Dom = <<"aaa.bbb.com">>,
     ThisNode = node(),
     AnotherNode = 'another@nohost',

test/event_pusher_sns_SUITE.erl

@@ -112,7 +112,7 @@ end_per_suite(_) ->
     ok.
 
 init_per_testcase(CaseName, Config) ->
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     meck:new(erlcloud_sns, [non_strict, passthrough]),
     meck:new([mongoose_wpool, mongoose_metrics], [stub_all]),
     meck:expect(erlcloud_sns, new, fun(_, _, _) -> mod_aws_sns_SUITE_erlcloud_sns_new end),

test/gen_hook_SUITE.erl

@@ -33,7 +33,7 @@ end_per_suite(Config) ->
     Config.
 
 init_per_testcase(_, Config) ->
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     Config.
 
 end_per_testcase(_, Config) ->
@@ -338,4 +338,4 @@ get_hook_handler(ModName, FunName, Fun) when is_function(Fun, 3) ->
     fun ModName:FunName/3.
 
 get_handlers_for_all_hooks() ->
-    ets:tab2list(gen_hook).
+    maps:to_list(persistent_term:get(gen_hook, #{})).

test/keystore_SUITE.erl

@@ -27,7 +27,7 @@ end_per_suite(_C) ->
 
 init_per_testcase(_, Config) ->
     mock_mongoose_metrics(),
-    Config1 = async_helper:start(Config, gen_hook, start_link, []),
+    Config1 = async_helper:start(Config, mongooseim_helper, start_link_loaded_hooks, []),
     mongoose_config:set_opts(opts()),
     Config1.
 

test/mod_global_distrib_SUITE.erl

@@ -68,7 +68,7 @@ init_per_testcase(_CaseName, Config) ->
     mongoose_config:set_opts(opts()),
     mongoose_domain_sup:start_link(),
     mim_ct_sup:start_link(ejabberd_sup),
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     mongoose_modules:start(),
     Config.
 

test/mongoose_cleanup_SUITE.erl

@@ -82,7 +82,7 @@ end_per_group(_Group, Config) ->
 
 init_per_testcase(TestCase, Config) ->
     mim_ct_sup:start_link(ejabberd_sup),
-    {ok, _HooksServer} = gen_hook:start_link(),
+    {ok, _HooksServer} = mongooseim_helper:start_link_loaded_hooks(),
     {ok, _DomainSup} = mongoose_domain_sup:start_link(),
     setup_meck(meck_mods(TestCase)),
     start_component(TestCase),

test/mongoose_subdomain_core_SUITE.erl

@@ -39,7 +39,7 @@ init_per_testcase(TestCase, Config) ->
     %%   - one "dynamic" host type with two configured domain names
     %% initial mongoose_subdomain_core conditions:
     %%   - no subdomains configured for any host type
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     mongoose_domain_sup:start_link(?STATIC_PAIRS, ?ALLOWED_HOST_TYPES),
     [mongoose_domain_core:insert(Domain, ?DYNAMIC_HOST_TYPE2, dummy_source)
      || Domain <- ?DYNAMIC_DOMAINS],

test/mongooseim_helper.erl

@@ -0,0 +1,7 @@
+-module(mongooseim_helper).
+-compile([export_all, nowarn_export_all]).
+
+start_link_loaded_hooks() ->
+    Ret = gen_hook:start_link(),
+    gen_hook:reload_hooks(),
+    Ret.

test/mongooseim_metrics_SUITE.erl

@@ -38,7 +38,7 @@ init_per_suite(C) ->
     Sup = spawn(fun() ->
         mim_ct_sup:start_link(ejabberd_sup),
         Hooks = {gen_hook,
-                 {gen_hook, start_link, []},
+                 {mongooseim_helper, start_link_loaded_hooks, []},
                  permanent,
                  brutal_kill,
                  worker,

test/muc_light_SUITE.erl

@@ -56,7 +56,7 @@ init_per_testcase(codec_calls, Config) ->
     ok = mnesia:create_schema([node()]),
     ok = mnesia:start(),
     {ok, _} = application:ensure_all_started(exometer_core),
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     ejabberd_router:start_link(),
     mim_ct_sup:start_link(ejabberd_sup),
     mongoose_modules:start(),

test/privacy_SUITE.erl

@@ -46,7 +46,7 @@ end_per_suite(_C) ->
     ok.
 
 init_per_testcase(_, C) ->
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     mongoose_config:set_opts(opts()),
     mongoose_modules:start(),
     C.

test/roster_SUITE.erl

@@ -48,7 +48,7 @@ end_per_suite(_C) ->
 
 init_per_testcase(_TC, C) ->
     init_ets(),
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     mongoose_modules:start(),
     C.
 

test/router_SUITE.erl

@@ -38,7 +38,7 @@ end_per_suite(_C) ->
 
 init_per_group(routing, Config) ->
     mongoose_config:set_opts(opts()),
-    gen_hook:start_link(),
+    mongooseim_helper:start_link_loaded_hooks(),
     ejabberd_router:start_link(),
     Config.