sendAndAwaitMessage not handling concurrent requests/responses correctly

[Philipp Buluschek]

V.0.7.5

When using XmppSession.sendAndAwaitMessage(Message, Predicate<Message>) to send a given message to several contacts, using the same Predicate for answer, all AsyncResult complete with the answer of the first answer received.

Expected: each answer should be the answers from the respective target of the message.

Example: <pre>

        Collection<Contact> contacts = new ArrayList<>();
        contacts.add(new Contact(Jid.of("[e-mail]")));
        contacts.add(new Contact(Jid.of("[e-mail]")));

        Predicate<Message> acceptResponse = m -> m.getBody().startsWith("Hello");

        List<AsyncResult<Message>> results = new ArrayList<>();
        for (Contact contact : contacts) {
            Message msg = new Message(contact.getJid(), Message.Type.CHAT, "Bonjour");
            AsyncResult<Message> sendAndAwaitMessage = xmppClient.sendAndAwaitMessage(msg, acceptResponse);
            results.add(sendAndAwaitMessage);
        }

        for (AsyncResult<Message> asyncResult : results) {
            try{
                Message msg = asyncResult.getResult();
                System.out.println(msg.getFrom() + " " + msg.getBody());
                // Actually prints 
                // [e-mail]/resource Hello from Alice
                // [e-mail]/resource Hello from Alice

                // Expected to print
                // [e-mail]/resource Hello from Alice
                // [e-mail]/resource Hello from Bob
            } catch(Exception e){
                System.out.println(e);
            }
        }

I assume that this is so, because the listener that is added in XmppSession.sendAndAwait(...) does not discriminate on the incoming Jid.

[Christian Schudt]

The behavior is as expected here. This is because the predicate only checks for the message body, which is true for both answers (from both contacts). So the first "Hello" message will satisfy the predicate and therefore complete both AsyncResults.

To "solve" this issue we would need to always match the "to" with the "from" attribute of sent and received messages in addition to any predicate, but I want to keep this decision to the predicate/developer. E.g. in case of groupchat "to" and "from" rarely match (to = bare Jid of room, from = full Jid of room occupant).

So my suggestion is to use a stricter and different predicate for each contact. E.g. use a "base" predicate and concat it with acceptResponse.and(m -> m.getFrom().equals(contactJid));

[Philipp Buluschek]

I understand the choice for flexibility, but it also does not respond to principle of least astonishment. When you sendAndAwaitMessage(), you expect the answer to be from the actual receipient not "any message which happens to come in at that time, and fits the predicate".

I see two possibilities to reduce that astonishment: 1. split the sendAndAwait in two methods, one which checks for the bare JID (to=from) and one which doesn't. Method names could be sendAndAwaitPersonalResponse() and sendAndAwaitGroupResponse() 2. or propose a predefined predicate to simplify the check

My preference goes towards 1.

In the meantime, I'm doing the check myself as you propose. (note that you actually need to compare bare JID, not the JID directly)

[Philipp Buluschek]

I understand Christian's comment as "won't fix". So closing to keep the backlog small.

[Christian Schudt]

Actually, I thought we could change the behavior as you proposed, but keep it simple and only use one method name. The only code which uses this function (within the library) is when changing a room subject in MUC (see https://xmpp.org/extensions/xep-0045.html#subject-mod).

to = bare JID of room from = full JID of occupant

Do you think it's conform to the principle of astonishment, when checking only the bare JID?

[Christian Schudt]

Do check the 'from' attribute of Message responses in sendAndAwaitMessage()

Don't violate the principle of least astonishment.

Fixes issue #136

→ <<cset 65ce3e079c4e>>

[Christian Schudt]

Fixed with 0.8.0