Make mod_persistent_lobby play well with breakout rooms #13939
Conversation
|
Hi, thanks for your contribution! |
There was a problem hiding this comment.
With this change, mod_persistent_lobby no longer breaks breakout rooms, but leaving the room destruction to breakout room module does mean it might prematurely kill main room while lobby still active.
So for full compatibility, breakout room will also need to check if persistent_lobby is enabled and if lobby is empty before triggering room deletion.
But this incompatibility is already the case right now, so should not block this PR. Merely suggesting follow up work for better integration.
|
Thanks @shawnchin @saghul for the feedback! I have made the changes requested. |
|
jenkins, test this please. |
|
@shawnchin WDYT? |
|
@damencho PR looks good with the latest changes. But as mentioned in my comments above, for full compatibility we might want to follow up with a change to update breakout mod so it doesn't kill room if persistent lobby enabled and lobby not empty. |
|
@dnna mind making that change? And the check, I think, should go there: or on the if above ... |
|
We might also need to check if persistent lobby enabled and lobby not empty? Otherwise, there's nothing in place to clean up the room in the case where main room is empty and everyone leaves the breakout room. Can easily check if persistent lobby is enabled using @damencho would it be simpler to have breakout module trigger event and let persistent lobby module handle it? E.g. in breakout module if main_room and main_room.close_timer then
if main_room._data.persist_lobby == true then
-- hand over deletion responsibility to mod_persistent_lobby
prosody.events.fire_event("persistent-lobby-check-occupancy", { room = main_room; });
else
module:log('info', 'Closing conference %s as all left for good.', main_room_jid);
main_room:set_persistent(false);
main_room:destroy(nil, 'All occupants left.');
end
endThen in persistent lobby module, we can implement the event handler to trigger occupancy check for both main room and lobby room before potentially deleting the room. |
|
Untested draft of how the event handler in mod_persistent_lobby might look like: function handle_persistent_lobby_occupancy_check(event)
--- Triggers occupancy check of main room and lobby room
local main_room = event.room;
local main_room_active = main_room and (main_room:has_occupant() or main_room._data.breakout_rooms_active);
-- nothing to do if persistent lobby not enabled or main room still active
if not has_persistent_lobby(main_room) or main_room_active then
return;
end
local lobby_room_jid = main_room._data.lobbyroom;
local lobby_room = lobby_muc_service.get_room_from_jid(lobby_room_jid);
-- if lobby empty (and we already checked that main room inactive) then room can be deleted
if not lobby_room:has_occupants() then
trigger_room_destroy(main_room);
end
end
module:hook_global('persistent-lobby-check-occupancy', handle_persistent_lobby_occupancy_check); |
|
We better not tight them up ... as by default breakout rooms are enabled, but persistent lobby is not. Its better breakout to fire event to say destroy this room. And breakout will be handling that event by itself, no change in current logic. But in case of persistent lobby can get that event do checks and stop any further propagation of the event so breakout room module will not destroy it. |
|
Hey @damencho @shawnchin, sorry for the late response. If it's possible, I'd prefer to split the changes to I think this PR already gives good value, as it resolves a scenario that is likely to occur for anyone who uses mod_persistent_lobby with breakout rooms. The other scenario exists, but to my understanding is more of an edge case (everyone jumps in the breakout rooms and leaves main room empty, while at the same time there is someone in the lobby). I think its fine to resolve at a later time. |
|
I had a close look at the issue, and it looks like we indeed need to do more to make persistent_lobby play well with breakout rooms, and vice versa. At present, both breakout room and persistent lobby modules attempt to handle lifecycle of main room without being aware of each other. @dnna Your fix effectively disables persistent lobby the moment a breakout room is created since As for the actual fix, I like @damencho 's suggestion that we trigger an event instead of immediately delete so another module has a chance to override it. But we will need to do this both ways. P.S. Once a proper fix is in place, we will need to undo this change so persistent lobby will continue to work even if breakout rooms were created |
|
I believe this PR is superceded by #14044 and can be closed. |
mod_persistent_lobbydoes not currently work well with breakout rooms. If everyone leaves the main room to join breakouts, then thetrigger_room_destroylogic is triggered from this plugin. This causes a crash in prosody with a hard-to-trace error like the below:This PR attempts to address this by checking whether there are active breakout rooms in the session, and if not basically letting mod_muc_breakout_rooms do its thing when it comes to closing the conference (see relevant code below):
https://github.com/jitsi/jitsi-meet/blob/stable/jitsi-meet_8960/resources/prosody-plugins/mod_muc_breakout_rooms.lua#L431
If breakout rooms are not active/not used, then mod_persistent_lobby handles destroying the conference as normal when both the lobby and the main room are empty.