MUC CleanupTask does not remove empty rooms in clustered environment correctly

I believe that I have encountered a bug in MultiUserChatServiceImpl.CleanupTask. I am using Openfire 4.2.3 in two-nodes clustered environment. The scenario was little bit complicated, but essentially both Openfire nodes got into a situation that an empty room in MUC was not removed (LocalMUCRoom.leaveRoom() detected that the user was not the originator, so it did not call mucService.removeChatRoom(name), but it set the empty room with an emptyDate.) When the CleanupTask kicked in, the senior member called localMUCRoomManager.cleanupRooms() which only removed the local cached MUCRoom. But the MUCRoom in the non-senior members were not removed. In the Openfire Console, the senior member showed all empty rooms removed, but the non-senior member still showed the empty rooms.

Here is a suggested fix in CleanupTask:

    private class CleanupTask extends TimerTask {
        @Override
        public void run() {
            if (ClusterManager.isClusteringStarted() && !ClusterManager.isSeniorClusterMember()) {
                // Do nothing if we are in a cluster and this JVM is not the senior cluster member
                return;
            }
            try {
                Date cleanUpDate = getCleanupDate();
                Iterator<LocalMUCRoom> it = localMUCRoomManager.getRooms().iterator();
                while (it.hasNext()) {
                    LocalMUCRoom room = it.next();
                    Date emptyDate = room.getEmptyDate();
                    if (emptyDate != null && emptyDate.before(cleanUpDate)) {
                        removeChatRoom(room.getName());
                    }
                }
            }
            catch (Throwable e) {
                Log.error(LocaleUtils.getLocalizedString("admin.error"), e);
            }
        }
    }

I didn’t make changes to LocalMUCRoomManager.cleanupRooms() because I didn’t want LocalMUCRoomManager to call MultiUserChatServiceImpl.removeChatRoom() which calls LocalMUCRoomManager.removeRoom().

I also made a minor enhancement to allow the CLEANUP_FREQUENCY to be configurable, so I did not have to wait for a hour to see if the fix worked. I redefined CLEANUP_FREQUENCY as

    private static final long CLEANUP_FREQUENCY = 60;

And in the start() method:

        long cleanupFreq = JiveGlobals.getLongProperty(
            "xmpp.muc.cleanupFrequency.inMinutes", CLEANUP_FREQUENCY) * 60 * 1000;
        TaskEngine.getInstance().schedule(cleanupTask, cleanupFreq, cleanupFreq);

Hi,

Is there any chance you could raise a PR for this?

Thanks,

Greg

Hi Greg,

I will do a PR.

-Vincent

BTW, I didn’t remove LocalMUCRoomManager#cleanupRooms() or marked it as deprecated although this method is unsafe to use.

-Vincent