From ab98e9a891ed8b3b3fe57723f4bcf147dabd7c77 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Wed, 8 Sep 2021 14:52:32 +0200 Subject: [PATCH 1/4] Add interface to accumulate affiliations Very often, the affiliations of a room will need to be checked many times. Imagine a room of a thousand people, and one sending a message: if any handler on user_send_packet needs to know the affiliations, they would be then queried again when the router triggers the process_packet callback for muclight. Such a handler happens often when we're checking if we can access the room, like in the case of smart_markers: then we could be introducing a race condition, when a user can react in a certain way in his user_send_packet hook, but then muclight finds out the user has just been kicked out. This can also be used as an optimisation: inbox is making a DB query for each user when muc_light routes the package to them, but we could be making a single DB call in the context of the sender with all the receivers queries at once, hence reducing round-trips considerably. --- src/muc_light/mod_muc_light.erl | 18 ++++++++++++++++++ src/muc_light/mod_muc_light_room.erl | 9 ++++----- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/muc_light/mod_muc_light.erl b/src/muc_light/mod_muc_light.erl index 0d92ee375c1..c37c47722f3 100644 --- a/src/muc_light/mod_muc_light.erl +++ b/src/muc_light/mod_muc_light.erl @@ -52,6 +52,7 @@ process_iq_set/4, is_muc_room_owner/4, can_access_room/4, + get_room_affiliations/2, can_access_identity/4]). %% For propEr @@ -531,6 +532,23 @@ can_access_room(true, _HostType, _Room, _User) -> can_access_room(_, _HostType, Room, User) -> none =/= get_affiliation(Room, User). +-spec get_room_affiliations(mongoose_acc:t(), jid:jid()) -> + {mongoose_acc:t(), {ok, aff_users(), binary()} | {error, not_exists}}. +get_room_affiliations(Acc1, Room) -> + case mongoose_acc:get(?MODULE, affiliations, undefined, Acc1) of + undefined -> + case mod_muc_light_db_backend:get_aff_users(jid:to_lus(Room)) of + {error, not_exists} -> + Acc2 = mongoose_acc:set(?MODULE, affiliations, {error, not_exists}, Acc1), + {Acc2, {error, not_exists}}; + {ok, _Affs, _Version} = Res -> + Acc2 = mongoose_acc:set_permanent(?MODULE, affiliations, Res, Acc1), + {Acc2, Res} + end; + Affs -> + {Acc1, Affs} + end. + -spec can_access_identity(Acc :: boolean(), HostType :: mongooseim:host_type(), Room :: jid:jid(), User :: jid:jid()) -> boolean(). diff --git a/src/muc_light/mod_muc_light_room.erl b/src/muc_light/mod_muc_light_room.erl index 0cc9b77f045..b8e9edde980 100644 --- a/src/muc_light/mod_muc_light_room.erl +++ b/src/muc_light/mod_muc_light_room.erl @@ -37,7 +37,6 @@ {?MOD_MUC_LIGHT_DB_BACKEND_BACKEN, modify_aff_users, 4}, {?MOD_MUC_LIGHT_DB_BACKEND_BACKEN, set_config, 3}, {?MOD_MUC_LIGHT_DB_BACKEND_BACKEN, destroy_room, 1}, - {?MOD_MUC_LIGHT_DB_BACKEND_BACKEN, get_aff_users, 1}, {?MOD_MUC_LIGHT_CODEC_BACKEND_BACKEN, encode, 5}, {?MOD_MUC_LIGHT_CODEC_BACKEND_BACKEN, encode_error, 5} ]). @@ -54,11 +53,11 @@ -spec handle_request(From :: jid:jid(), RoomJID :: jid:jid(), OrigPacket :: exml:element(), Request :: muc_light_packet(), Acc :: mongoose_acc:t()) -> mongoose_acc:t(). -handle_request(From, To, OrigPacket, Request, Acc) -> +handle_request(From, To, OrigPacket, Request, Acc1) -> RoomUS = jid:to_lus(To), - AffUsersRes = mod_muc_light_db_backend:get_aff_users(RoomUS), - Response = process_request(From, RoomUS, Request, AffUsersRes, Acc), - send_response(From, To, RoomUS, OrigPacket, Response, Acc). + {Acc2, AffUsersRes} = mod_muc_light:get_room_affiliations(Acc1, To), + Response = process_request(From, RoomUS, Request, AffUsersRes, Acc2), + send_response(From, To, RoomUS, OrigPacket, Response, Acc2). -spec maybe_forget(Acc :: mongoose_acc:t(), RoomUS :: jid:simple_bare_jid(), NewAffUsers :: aff_users()) -> any(). From e28a0c1b854409ec8d05ef1518a61973e160b211 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Wed, 8 Sep 2021 15:12:05 +0200 Subject: [PATCH 2/4] Create the same interface for classic muc --- src/mod_muc.erl | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/mod_muc.erl b/src/mod_muc.erl index ec138866901..b571562701c 100644 --- a/src/mod_muc.erl +++ b/src/mod_muc.erl @@ -62,6 +62,7 @@ %% Hooks handlers -export([is_muc_room_owner/4, can_access_room/4, + get_room_affiliations/2, can_access_identity/4, disco_local_items/1]). @@ -82,7 +83,7 @@ {?MOD_MUC_DB_BACKEND, set_nick, 4}, {?MOD_MUC_DB_BACKEND, store_room, 4}, {?MOD_MUC_DB_BACKEND, unset_nick, 3}, - can_access_identity/4, can_access_room/4, create_instant_room/6, + can_access_identity/4, can_access_room/4, get_room_affiliations/2, create_instant_room/6, disco_local_items/1, hibernated_rooms_number/0, is_muc_room_owner/4, online_rooms_number/0, register_room/4, restore_room/3, start_link/2 ]). @@ -1251,6 +1252,23 @@ can_access_room(_, _HostType, Room, User) -> {ok, CanAccess} -> CanAccess end. +-spec get_room_affiliations(mongoose_acc:t(), jid:jid()) -> + {mongoose_acc:t(), any()}. +get_room_affiliations(Acc1, Room) -> + case mongoose_acc:get(?MODULE, affiliations, undefined, Acc1) of + undefined -> + case mod_muc_room:get_room_users(Room) of + {error, not_found} -> + Acc2 = mongoose_acc:set(?MODULE, affiliations, {error, not_found}, Acc1), + {Acc2, {error, not_found}}; + {ok, _Affs} = Res -> + Acc2 = mongoose_acc:set_permanent(?MODULE, affiliations, Res, Acc1), + {Acc2, Res} + end; + Affs -> + {Acc1, Affs} + end. + -spec can_access_identity(Acc :: boolean(), HostType :: mongooseim:host_type(), Room :: jid:jid(), User :: jid:jid()) -> boolean(). From 3e60be181916a801bf4fe2aca56afd9232e8dd74 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Wed, 8 Sep 2021 15:14:58 +0200 Subject: [PATCH 3/4] Make get_room_affiliations a hook --- src/mod_muc.erl | 1 + src/mongoose_hooks.erl | 13 ++++++++++++- src/muc_light/mod_muc_light.erl | 1 + 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/mod_muc.erl b/src/mod_muc.erl index b571562701c..65414a7c63b 100644 --- a/src/mod_muc.erl +++ b/src/mod_muc.erl @@ -1340,6 +1340,7 @@ config_metrics(HostType) -> hooks(HostType) -> [{is_muc_room_owner, HostType, ?MODULE, is_muc_room_owner, 50}, {can_access_room, HostType, ?MODULE, can_access_room, 50}, + {get_room_affiliations, HostType, ?MODULE, get_room_affiliations, 50}, {can_access_identity, HostType, ?MODULE, can_access_identity, 50}, {disco_local_items, HostType, ?MODULE, disco_local_items, 250}]. diff --git a/src/mongoose_hooks.erl b/src/mongoose_hooks.erl index b3ae6d840c6..61bcbfa5b7f 100644 --- a/src/mongoose_hooks.erl +++ b/src/mongoose_hooks.erl @@ -9,6 +9,8 @@ -include("mod_privacy.hrl"). -include("mongoose.hrl"). +-ignore_xref([get_room_affiliations/2]). + -export([adhoc_local_commands/4, adhoc_sm_commands/4, anonymous_purge_hook/3, @@ -84,7 +86,8 @@ -export([is_muc_room_owner/3, can_access_identity/3, - can_access_room/3]). + can_access_room/3, + get_room_affiliations/2]). -export([mam_archive_id/2, mam_archive_size/3, @@ -899,6 +902,14 @@ can_access_identity(HostType, Room, User) -> can_access_room(HostType, Room, User) -> run_hook_for_host_type(can_access_room, HostType, false, [HostType, Room, User]). +-spec get_room_affiliations(Acc, Room) -> Result when + Acc :: mongoose_acc:t(), + Room :: jid:jid(), + Result :: {mongoose_acc:t(), term()}. +get_room_affiliations(Acc, Room) -> + HostType = mongoose_acc:host_type(Acc), + run_hook_for_host_type(get_room_affiliations, HostType, Acc, [Room]). + %% MAM related hooks %%% @doc The `mam_archive_id' hook is called to determine diff --git a/src/muc_light/mod_muc_light.erl b/src/muc_light/mod_muc_light.erl index c37c47722f3..2672cb95a11 100644 --- a/src/muc_light/mod_muc_light.erl +++ b/src/muc_light/mod_muc_light.erl @@ -300,6 +300,7 @@ hooks(HostType) -> Roster = gen_mod:get_module_opt(HostType, ?MODULE, rooms_in_rosters, ?DEFAULT_ROOMS_IN_ROSTERS), [{is_muc_room_owner, HostType, ?MODULE, is_muc_room_owner, 50}, {can_access_room, HostType, ?MODULE, can_access_room, 50}, + {get_room_affiliations, HostType, ?MODULE, get_room_affiliations, 50}, {can_access_identity, HostType, ?MODULE, can_access_identity, 50}, %% Prevent sending service-unavailable on groupchat messages {offline_groupchat_message_hook, HostType, ?MODULE, prevent_service_unavailable, 90}, From 0957070e13ec22e860913e89ab79f6302fd6db0b Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Wed, 8 Sep 2021 15:30:02 +0200 Subject: [PATCH 4/4] Use the affiliations interface in one more place --- src/muc_light/mod_muc_light.erl | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/muc_light/mod_muc_light.erl b/src/muc_light/mod_muc_light.erl index 2672cb95a11..e239f519935 100644 --- a/src/muc_light/mod_muc_light.erl +++ b/src/muc_light/mod_muc_light.erl @@ -171,13 +171,12 @@ try_to_create_room(CreatorUS, RoomJID, #create{raw_config = RawConfig} = Creatio Acc :: mongoose_acc:t()) -> {ok, jid:jid(), config_req_props()} | {error, validation_error() | bad_request | not_allowed}. -change_room_config(UserJid, RoomID, MUCLightDomain, ConfigReq, Acc) -> +change_room_config(UserJid, RoomID, MUCLightDomain, ConfigReq, Acc1) -> R = {RoomID, MUCLightDomain}, RoomJID = jid:make(RoomID, MUCLightDomain, <<>>), RoomUS = jid:to_lus(RoomJID), - AffUsersRes = mod_muc_light_db_backend:get_aff_users(RoomUS), - - case mod_muc_light_room:process_request(UserJid, R, {set, ConfigReq}, AffUsersRes, Acc) of + {Acc2, AffUsersRes} = mod_muc_light:get_room_affiliations(Acc1, RoomJID), + case mod_muc_light_room:process_request(UserJid, RoomUS, {set, ConfigReq}, AffUsersRes, Acc2) of {set, ConfigResp, _} -> {ok, RoomJID, ConfigResp}; {error, _Reason} = E ->