Merge pull request #3775 from esl/remove_domain/catch_hook_errors

Remove domain/catch hook errors

big_tests/tests/domain_removal_SUITE.erl

@@ -25,7 +25,8 @@ all() ->
      {group, offline_removal},
      {group, markers_removal},
      {group, vcard_removal},
-     {group, last_removal}
+     {group, last_removal},
+     {group, removal_failures}
     ].
 
 groups() ->
@@ -43,7 +44,8 @@ groups() ->
      {offline_removal, [], [offline_removal]},
      {markers_removal, [], [markers_removal]},
      {vcard_removal, [], [vcard_removal]},
-     {last_removal, [], [last_removal]}
+     {last_removal, [], [last_removal]},
+     {removal_failures, [], [removal_stops_if_handler_fails]}
     ].
 
 %%%===================================================================
@@ -80,6 +82,8 @@ end_per_group(_Groupname, Config) ->
     end,
     ok.
 
+group_to_modules(removal_failures) ->
+    group_to_modules(mam_removal);
 group_to_modules(auth_removal) ->
     [];
 group_to_modules(cache_removal) ->
@@ -394,25 +398,63 @@ last_removal(Config0) ->
 
             PresUn = escalus_client:wait_for_stanza(Alice),
             escalus:assert(is_presence_with_type, [<<"unavailable">>], PresUn),
-    
+
             %% Alice asks for Bob's last availability
             BobShortJID = escalus_client:short_jid(Bob),
             GetLast = escalus_stanza:last_activity(BobShortJID),
             Stanza = escalus_client:send_iq_and_wait_for_result(Alice, GetLast),
-    
+
             %% Alice receives Bob's status and last online time > 0
             escalus:assert(is_last_result, Stanza),
             true = (1 =< get_last_activity(Stanza)),
             <<"I am a banana!">> = get_last_status(Stanza),
-    
-            run_remove_domain(),                                         
+
+            run_remove_domain(),
             escalus_client:send(Alice, GetLast),
             Error = escalus_client:wait_for_stanza(Alice),
             escalus:assert(is_error, [<<"auth">>, <<"forbidden">>], Error)
         end,
     escalus:fresh_story_with_config(Config0, [{alice, 1}, {bob, 1}], F).
 
+removal_stops_if_handler_fails(Config0) ->
+    mongoose_helper:inject_module(?MODULE),
+    F = fun(Config, Alice) ->
+        start_domain_removal_hook(),
+        Room = muc_helper:fresh_room_name(),
+        MucHost = muc_light_helper:muc_host(),
+        muc_light_helper:create_room(Room, MucHost, alice, [], Config, muc_light_helper:ver(1)),
+        RoomAddr = <<Room/binary, "@", MucHost/binary>>,
+        escalus:send(Alice, escalus_stanza:groupchat_to(RoomAddr, <<"text">>)),
+        escalus:wait_for_stanza(Alice),
+        mam_helper:wait_for_room_archive_size(MucHost, Room, 1),
+        run_remove_domain(),
+        mam_helper:wait_for_room_archive_size(MucHost, Room, 1),
+        stop_domain_removal_hook(),
+        run_remove_domain(),
+        mam_helper:wait_for_room_archive_size(MucHost, Room, 0)
+        end,
+    escalus_fresh:story_with_config(Config0, [{alice, 1}], F).
+
 %% Helpers
+start_domain_removal_hook() ->
+    rpc(mim(), ?MODULE, rpc_start_domain_removal_hook, [host_type()]).
+
+stop_domain_removal_hook() ->
+    rpc(mim(), ?MODULE, rpc_stop_domain_removal_hook, [host_type()]).
+
+rpc_start_domain_removal_hook(HostType) ->
+    gen_hook:add_handler(remove_domain, HostType,
+                         fun ?MODULE:domain_removal_hook_fn/3,
+                         #{}, 30). %% Priority is so that it comes before muclight and mam
+
+rpc_stop_domain_removal_hook(HostType) ->
+    gen_hook:delete_handler(remove_domain, HostType,
+                            fun ?MODULE:domain_removal_hook_fn/3,
+                            #{}, 30).
+
+domain_removal_hook_fn(Acc, _Params, _Extra) ->
+    F = fun() -> throw(first_time_needs_to_fail) end,
+    mongoose_domain_api:remove_domain_wrapper(Acc, F, ?MODULE).
 
 connect_and_disconnect(Spec) ->
     {ok, Client, _} = escalus_connection:start(Spec),

src/auth/ejabberd_auth.erl

@@ -425,12 +425,15 @@ auth_methods(HostType) ->
 auth_method_to_module(Method) ->
     list_to_atom("ejabberd_auth_" ++ atom_to_list(Method)).
 
--spec remove_domain(mongoose_hooks:simple_acc(), mongooseim:host_type(), jid:lserver()) ->
-          mongoose_hooks:simple_acc().
+-spec remove_domain(mongoose_domain_api:remove_domain_acc(), mongooseim:host_type(), jid:lserver()) ->
+    mongoose_domain_api:remove_domain_acc().
 remove_domain(Acc, HostType, Domain) ->
-    F = fun(Mod) -> mongoose_gen_auth:remove_domain(Mod, HostType, Domain) end,
-    call_auth_modules_for_host_type(HostType, F, #{op => map}),
-    Acc.
+    F = fun() ->
+            FAuth = fun(Mod) -> mongoose_gen_auth:remove_domain(Mod, HostType, Domain) end,
+            call_auth_modules_for_host_type(HostType, FAuth, #{op => map}),
+            Acc
+        end,
+    mongoose_domain_api:remove_domain_wrapper(Acc, F, ?MODULE).
 
 ensure_metrics(Host) ->
     Metrics = [authorize, check_password, try_register, does_user_exist],

src/domain/mongoose_domain_api.erl

@@ -2,6 +2,8 @@
 %% management.
 -module(mongoose_domain_api).
 
+-include("mongoose_logger.hrl").
+
 -export([init/0,
          stop/0,
          get_host_type/1]).
@@ -27,6 +29,9 @@
          get_subdomain_info/1,
          get_all_subdomains_for_domain/1]).
 
+%% Helper for remove_domain
+-export([remove_domain_wrapper/3]).
+
 %% For testing
 -export([get_all_dynamic/0]).
 
@@ -38,7 +43,9 @@
 -type domain() :: jid:lserver().
 -type host_type() :: mongooseim:host_type().
 -type subdomain_pattern() :: mongoose_subdomain_utils:subdomain_pattern().
--export_type([status/0]).
+-type remove_domain_acc() :: #{failed := [module()]}.
+
+-export_type([status/0, remove_domain_acc/0]).
 
 -spec init() -> ok | {error, term()}.
 init() ->
@@ -251,3 +258,15 @@ unregister_subdomain(HostType, SubdomainPattern) ->
           [mongoose_subdomain_core:subdomain_info()].
 get_all_subdomains_for_domain(Domain) ->
     mongoose_subdomain_core:get_all_subdomains_for_domain(Domain).
+
+-spec remove_domain_wrapper(remove_domain_acc(), fun(() -> remove_domain_acc()), module()) ->
+    remove_domain_acc() | {stop, remove_domain_acc()}.
+remove_domain_wrapper(Acc, F, Module) ->
+    try F()
+    catch C:R:S ->
+        ?LOG_ERROR(#{what => hook_failed,
+                     text => <<"Error running hook">>,
+                     module => Module,
+                     class => C, reason => R, stacktrace => S}),
+        {stop, Acc#{failed := [Module | maps:get(failed, Acc)]}}
+    end.

src/inbox/mod_inbox.erl

@@ -260,12 +260,14 @@ remove_user(Acc, User, Server) ->
     mod_inbox_utils:clear_inbox(HostType, User, Server),
     Acc.
 
--spec remove_domain(mongoose_hooks:simple_acc(),
-                    mongooseim:host_type(), jid:lserver()) ->
-    mongoose_hooks:simple_acc().
+-spec remove_domain(mongoose_domain_api:remove_domain_acc(), mongooseim:host_type(), jid:lserver()) ->
+    mongoose_domain_api:remove_domain_acc().
 remove_domain(Acc, HostType, Domain) ->
-    mod_inbox_backend:remove_domain(HostType, Domain),
-    Acc.
+    F = fun() ->
+            mod_inbox_backend:remove_domain(HostType, Domain),
+            Acc
+        end,
+    mongoose_domain_api:remove_domain_wrapper(Acc, F, ?MODULE).
 
 -spec disco_local_features(mongoose_disco:feature_acc(),
                            map(),

src/mam/mod_mam_muc_rdbms_arch.erl

@@ -312,15 +312,17 @@ remove_archive(Acc, HostType, ArcID, _ArcJID) ->
     mongoose_rdbms:execute_successfully(HostType, mam_muc_archive_remove, [ArcID]),
     Acc.
 
--spec remove_domain(mongoose_hooks:simple_acc(),
-                    mongooseim:host_type(), jid:lserver()) ->
-    mongoose_hooks:simple_acc().
+-spec remove_domain(mongoose_domain_api:remove_domain_acc(), host_type(), jid:lserver()) ->
+    mongoose_domain_api:remove_domain_acc().
 remove_domain(Acc, HostType, Domain) ->
-    SubHosts = get_subhosts(HostType, Domain),
-    {atomic, _} = mongoose_rdbms:sql_transaction(HostType, fun() ->
-            [remove_domain_trans(HostType, SubHost) || SubHost <- SubHosts]
-        end),
-    Acc.
+    F = fun() ->
+            SubHosts = get_subhosts(HostType, Domain),
+            {atomic, _} = mongoose_rdbms:sql_transaction(HostType, fun() ->
+                    [remove_domain_trans(HostType, SubHost) || SubHost <- SubHosts]
+                end),
+            Acc
+    end,
+    mongoose_domain_api:remove_domain_wrapper(Acc, F, ?MODULE).
 
 remove_domain_trans(HostType, MucHost) ->
     mongoose_rdbms:execute_successfully(HostType, mam_muc_remove_domain, [MucHost]),

src/mam/mod_mam_rdbms_arch.erl

@@ -341,15 +341,18 @@ remove_archive(Acc, HostType, ArcID, _ArcJID) ->
     mongoose_rdbms:execute_successfully(HostType, mam_archive_remove, [ArcID]),
     Acc.
 
--spec remove_domain(mongoose_hooks:simple_acc(), host_type(), jid:lserver()) ->
-    mongoose_hooks:simple_acc().
+-spec remove_domain(mongoose_domain_api:remove_domain_acc(), host_type(), jid:lserver()) ->
+    mongoose_domain_api:remove_domain_acc().
 remove_domain(Acc, HostType, Domain) ->
-    {atomic, _} = mongoose_rdbms:sql_transaction(HostType, fun() ->
-            mongoose_rdbms:execute_successfully(HostType, mam_remove_domain, [Domain]),
-            mongoose_rdbms:execute_successfully(HostType, mam_remove_domain_prefs, [Domain]),
-            mongoose_rdbms:execute_successfully(HostType, mam_remove_domain_users, [Domain])
-        end),
-    Acc.
+    F = fun() ->
+            {atomic, _} = mongoose_rdbms:sql_transaction(HostType, fun() ->
+                mongoose_rdbms:execute_successfully(HostType, mam_remove_domain, [Domain]),
+                mongoose_rdbms:execute_successfully(HostType, mam_remove_domain_prefs, [Domain]),
+                mongoose_rdbms:execute_successfully(HostType, mam_remove_domain_users, [Domain])
+            end),
+            Acc
+        end,
+    mongoose_domain_api:remove_domain_wrapper(Acc, F, ?MODULE).
 
 %% GDPR logic
 extract_gdpr_messages(Env, ArcID) ->

src/mongoose_hooks.erl

@@ -235,7 +235,7 @@ does_user_exist(HostType, Jid, RequestType) ->
 -spec remove_domain(HostType, Domain) -> Result when
     HostType :: binary(),
     Domain :: jid:lserver(),
-    Result :: #{failed => [module()]}.
+    Result :: mongoose_domain_api:remove_domain_acc().
 remove_domain(HostType, Domain) ->
     run_hook_for_host_type(remove_domain, HostType, #{failed => []}, [HostType, Domain]).
 

src/muc_light/mod_muc_light.erl

@@ -420,13 +420,15 @@ remove_user(Acc, User, Server) ->
             Acc
     end.
 
--spec remove_domain(mongoose_hooks:simple_acc(),
-                    mongooseim:host_type(), jid:lserver()) ->
-    mongoose_hooks:simple_acc().
+-spec remove_domain(mongoose_domain_api:remove_domain_acc(), mongooseim:host_type(), jid:lserver()) ->
+    mongoose_domain_api:remove_domain_acc().
 remove_domain(Acc, HostType, Domain) ->
-    MUCHost = server_host_to_muc_host(HostType, Domain),
-    mod_muc_light_db_backend:remove_domain(HostType, MUCHost, Domain),
-    Acc.
+    F = fun() ->
+            MUCHost = server_host_to_muc_host(HostType, Domain),
+            mod_muc_light_db_backend:remove_domain(HostType, MUCHost, Domain),
+            Acc
+        end,
+    mongoose_domain_api:remove_domain_wrapper(Acc, F, ?MODULE).
 
 -spec add_rooms_to_roster(Acc :: mongoose_acc:t(), UserJID :: jid:jid()) -> mongoose_acc:t().
 add_rooms_to_roster(Acc, UserJID) ->