Possible bug in RoutingTableImpl.routeToBareJID

Openfire Openfire Dev
1

Hi all,

I think I found a bug in the routing policy. In the documentation of RoutingTableImpl.routeToBareJID method (http://fisheye.igniterealtime.org/browse/openfiregit/src/java/org/jivesoftware/o penfire/spi/RoutingTableImpl.java?r=128395c93ed2ad541ec1e0b1efafaaba66541f5f&r=1 28395c93ed2ad541ec1e0b1efafaaba66541f5f#to518) is stated:

In the case that the user is connected from many resources the logic will be the

following:

  • Select resources with highest priority

  • Select resources with highest show value (chat, available, away, xa, dnd)

  • Select resource with most recent activity

But I think that under the following circumstances we send the message to a random resource even if it has a negative priority:

  • Let’s say we have 2 sessions (so 2 different resources jid@of.org/A and jid@of.org/B)
  • session.isMessageCarbonsEnabled() is null for both
  • the route.all-resources variable is set to false or null
  • the two resources have the same show value
  • the two resources have 2 different priorities -1 and 1

So under the above circumstances we never check the resource priority but only the resource with the most recent activity, hence if the resource with -1 was had the last activity we will send messages to it.

I hope everything is clear and I didn’t misunderstand something.

Thank you,

Antonio

2

Thanks for the report!

I think that is something that got overlooked when adding support for Message Carbons, because carbons do not care about the priority.

If you have a fix proposal ready, let us know please!

3

Hi,

it’s a one line change, we have to add this:

sessions = getHighestPrioritySessions(sessions);

here: http://fisheye.igniterealtime.org/browse/openfiregit/src/java/org/jivesoftware/o penfire/spi/RoutingTableImpl.java?r=128395c93ed2ad541ec1e0b1efafaaba66541f5f&r=1 28395c93ed2ad541ec1e0b1efafaaba66541f5f#to549

Thank you,

Antonio

4

This is what changed with message carbons:

Add support for XEP-0280 Message Carbons · igniterealtime/Openfire@128395c · GitHub ba66541f5f#diff-42211d9b12f65e2f7e539cc97c0308afL486

Can you have a look please?

It was in line 495 previously.

The problem is, Openfire only delivers to highest priority sessions, but XEP-280 (Message Carbons) and RFC 6121 (8.5.2.1.1. Message) also allows to deliver to other non-negative sessions:

If there is more than one resource with a non-negative presence priority then the server MUST either (a) deliver the message to the “most available” resource or resources (according to the server’s implementation-specific algorithm, e.g., treating the resource or resources with the highest presence priority as “most available”) or (b) deliver the message to all of the non-negative resources that have opted in to receive chat messages.

Openfire has no logic for b), but only for a), as far as I can see.

For Message Carbons we would need b) (otherwise carbons wouldn’t make much sense).

Does anyone think it’s acceptable to restore line 495 and make the getHighestPrioritySessions return all non-negative sessions?

It would also probably make the route.all-resources more understandable.

5

Hi,

we could change the following:

privateListgetHighestPrioritySessions(Listsessions){

int highest = Integer.MIN_VALUE;

// Get the highest priority amongst the sessions

for (ClientSession session : sessions) {

int priority = session.getPresence().getPriority();

if (priority >= 0 && priority > highest) {

highest = priority;

}

}

// Answer an empty collection if all have negative priority

if (highest == Integer.MIN_VALUE) {

return Collections.emptyList();

}

// Get sessions that have the highest priority

List answer = new ArrayList(sessions.size());

for (ClientSession session : sessions) {

if (session.getPresence().getPriority() == highest) {

answer.add(session);

}

}

return answer;

}

to:

privateListgetHighestPrioritySessions(Listsessions){

int highest = Integer.MIN_VALUE;

// Get the highest priority amongst the sessions

for (ClientSession session : sessions) {

int priority = session.getPresence().getPriority();

if (priority >= 0 && priority > highest) {

highest = priority;

}

}

// Get sessions that have the highest priority

return highPassFilterPrioritySessions(sessions, highest);

}

private List highPassFilterPrioritySessions(

                                                                       List<ClientSession> sessions,

                                                                       int min) {

if (min < 0)

        return Collections.emptyList();       

    // Get sessions with priority >= min

    List<ClientSession> answer = new ArrayList<ClientSession>(sessions.size());

    for (ClientSession session : sessions)

if (session.getPresence().getPriority() >= min)

answer.add(session);

return answer;

}

and we can add:

sessions = highPassFilterPrioritySessions(sessions, highest);

here GitHub - igniterealtime/Openfire at 128395c93ed2ad541ec1e0b1efafaaba66541f5f 66541f5f/src/java/org/jivesoftware/openfire/spi/RoutingTableImpl.java#L527

Thank you,

Antonio

6

Any update on this?

Thank you,

Antonio

7

I’ve filed a bug for it:

OF-818

Will look into it, when time permits.

8

I’ve created a bug fix for it. I also renamed your method and made sure that message carbons are also sent to non-negative resources, which are not among the highest.

If you want, you can review it:

https://github.com/sco0ter/Openfire/commit/48676c7c20dd11e423741cbf2ecaf0f5d1694 e27

If you have concerns about the functionality, please let me know.

9

Looks good to me, I would only move the .isEmpty() check from line 542 to line 530, on nonNegativePrioritySessions list instead of the highestPrioritySessions list, because if it is empty we can return false without instantiating other objects. What do you think?

Thank you,

Antonio

10

Thanks for the hint, makes sense!

Like so?

https://github.com/sco0ter/Openfire/commit/20d4820d919f15e70203e33feb7071b2c836d 300

11

It’s exactly what I meant.

Thank you,

Antonio