Provide state in MUC room hooks: start/destroyed/join/leave

[ggyurchev]

Is your feature request related to a problem? Please describe.
Since every MUC room runs in its own process, hooks, originating from mod_muc_room cannot access the state data directly unless it's provided as parameter. It becomes either hacky or impossible to implement external modules without modifying and recompiling the ejabberd code.

Describe the solution you'd like

My suggestion is to add the state data to the following hooks: start_room, room_destroyed, join_room, leave_room

Describe alternatives you've considered

There are a few alternatives depending on the hook but none are as clean as having the state data directly:

1. muc_filter_presence can be used to detect a new room (if not created by mod_muc_admin)
2. A new process can be spawned and MUC state retrieved there. This is not always ideal in terms of synchronization and optimization.

Additional context

One use case we have specifically is calling an external API with some info about the room when someone leaves or joins. Also, it's important for us to know who the room creator is when creating the room. Currently, start_room does not provide that.

[badlop]

I tried adding a function to the hook, which calls the room pid to get the state... That doesn't work, probably because the process running the room is the same one that runs the hook, and the function itself.

From what I see, hooks with accumulator usually provide the state variable because the hooked functions will modify the state and return it.

On the other hand, hooks without accumulator provide specific arguments, not whole state record, I imagine because those state records are small and providing specific arguments is enough, no need for the whole state.

In this case, as the room state has many information, it makes sense to provide all the state record, which is defined in mod_muc_room.hrl.

The change is simple, and right now it doesn't affect ejabberd, only mod_tombstones in ejabberd-contrib.

From bfe577117c2d5c26c6ce990df21091da40fd11d4 Mon Sep 17 00:00:00 2001
From: Badlop <[email protected]>
Date: Tue, 19 Nov 2024 12:41:28 +0100
Subject: [PATCH 1/2] Add State to room start/destroy/join/leave hooks (4308)

---
 src/mod_muc_room.erl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/mod_muc_room.erl b/src/mod_muc_room.erl
index 85fb54249..fbe759822 100644
--- a/src/mod_muc_room.erl
+++ b/src/mod_muc_room.erl
@@ -309,7 +309,7 @@ init([Host, ServerHost, Access, Room, HistorySize,
 	      [Room, Host, jid:encode(Creator)]),
     add_to_log(room_existence, created, State1),
     add_to_log(room_existence, started, State1),
-    ejabberd_hooks:run(start_room, ServerHost, [ServerHost, Room, Host]),
+    ejabberd_hooks:run(start_room, ServerHost, [ServerHost, Room, Host, State1]),
     erlang:send_after(?CLEAN_ROOM_TIMEOUT, self(),
                       close_room_if_temporary_and_empty),
     {ok, normal_state, reset_hibernate_timer(State1)};
@@ -328,7 +328,7 @@ init([Host, ServerHost, Access, Room, HistorySize, RoomShaper, Opts, QueueType])
 				  room_queue = RoomQueue,
 				  room_shaper = Shaper}),
     add_to_log(room_existence, started, State),
-    ejabberd_hooks:run(start_room, ServerHost, [ServerHost, Room, Host]),
+    ejabberd_hooks:run(start_room, ServerHost, [ServerHost, Room, Host, State]),
     State1 = cleanup_affiliations(State),
     State2 =
     case {lists:keyfind(hibernation_time, 1, Opts),
@@ -1016,9 +1016,9 @@ terminate(Reason, _StateName,
 		add_to_log(room_existence, stopped, StateData),
 		case (StateData#state.config)#config.persistent of
                     false ->
-			ejabberd_hooks:run(room_destroyed, LServer, [LServer, Room, Host, false]);
+			ejabberd_hooks:run(room_destroyed, LServer, [LServer, Room, Host, false, StateData]);
                     {destroying, Persistent} ->
-			ejabberd_hooks:run(room_destroyed, LServer, [LServer, Room, Host, Persistent]);
+			ejabberd_hooks:run(room_destroyed, LServer, [LServer, Room, Host, Persistent, StateData]);
 		    _ ->
 			ok
 		end
@@ -5384,7 +5384,7 @@ tab_add_online_user(JID, StateData) ->
     Room = StateData#state.room,
     Host = StateData#state.host,
     ServerHost = StateData#state.server_host,
-    ejabberd_hooks:run(join_room, ServerHost, [ServerHost, Room, Host, JID]),
+    ejabberd_hooks:run(join_room, ServerHost, [ServerHost, Room, Host, JID, StateData]),
     mod_muc:register_online_user(ServerHost, jid:tolower(JID), Room, Host).
 
 -spec tab_remove_online_user(jid(), state()) -> any().
@@ -5392,7 +5392,7 @@ tab_remove_online_user(JID, StateData) ->
     Room = StateData#state.room,
     Host = StateData#state.host,
     ServerHost = StateData#state.server_host,
-    ejabberd_hooks:run(leave_room, ServerHost, [ServerHost, Room, Host, JID]),
+    ejabberd_hooks:run(leave_room, ServerHost, [ServerHost, Room, Host, JID, StateData]),
     mod_muc:unregister_online_user(ServerHost, jid:tolower(JID), Room, Host).
 
 -spec tab_count_user(jid(), state()) -> non_neg_integer().
-- 
2.43.0

[ggyurchev]

This is exactly what I have in mind and yes, the process is the same, so it creates a deadlock. Just thought it would be a nice addition to a future ejabberd version. Currently, our workaround is to do what we need asynchronously by spawning a new process.