MUC problem / Smack should use LinkedHashmap for PacketListeners

Hi,

I have the following problem with MUC:

I added a ParticipantListener to the MUC object in order to get presence updates for the MUC occupants.

The MUC class itself also adds a PacketListener (with presence filter) to listen for new room occupants, to populate the “occupantsMap” and to eventually fire the “joined” event.

In my ParticipantListener I want to use something like this:

Occupant occupant = multiUserChat.getOccupant(presence.getFrom());

in order to update the status of the occupant.

The problem now is:

Both listeners (MUC’s internal and mine) are called in random order. So sometimes multiUserChat.getOccupant(presence.getFrom()) returns null, if my listener is called prior to the internal one, because the occupantsMap is not yet filled.

I probably could work around this somehow by updating the presence directly from the joined method, but in this case I also the presence could be updated twice and I probably need to keep track if the user has just joined or if it is a later presence update.

Anyway I think the core problem is the logic in how Smack manages listeners:

protected final Map<PacketListener, ListenerWrapper> recvListeners = new ConcurrentHashMap<PacketListener, ListenerWrapper>();

for (ListenerWrapper listenerWrapper : connection.recvListeners.values()) {

            listenerWrapper.notifyListener(packet);

}

Iterating through the listeners values() results in random order listener notifications.

I think it is better to ensure listener execution in a predictable order, something like:

protected final Map<PacketListener, ListenerWrapper> recvListeners = Collections.synchronizedMap(new LinkedHashMap<PacketListener, ListenerWrapper>());

might solve the problem. Otherwise developers have to deal with randomness :frowning:

(Its only necessary, to add synchronized blocks around iterations of recvListeners.values() then).

We get that complaint a lot and it looks like your proposal using a ordered list may be solve the problem that some listeners should run before others. In fact, IMHO, Smack should gurantee that.

IIRC that problem also manifests in other classes that use that design pattern (e.g. Roster).

SMACK-532

It would make even more sense to store it as a List, since that actually guarentees order by contract. The map structure is only required to delete the listener, which can be easily accomplished by simply parsing the list.

If that proves to be too slow, then a secondary map can be used to store the location, but I doubt that would be necessary.