OF-2012: Remove, instead of add, a route when a session becomes unavailable

[guusdk]

No description provided.

[guusdk]

I was triggered by debug logging that I added as part of #1626 - I noticed that the 'put' to the routing table continuously failed, as the route was already there.

I'm assuming that this is an oversight that has lingered in the code for way to long. Given that it has been in the code for such a long time, it's probably pretty benign. My worry is that somehow, something now depends on this, which would make this PR potentially a breaking change.

[GregDThomas]

I don't think this is MR is right.

org.xmpp.packet.Presence#isAvailable() == true if there is a no type attribute of the presence packet after logging in.

So, org.jivesoftware.openfire.SessionManager#sessionUnavailable is called if a presence packet is received without a type attribute after more recently receiving one with.

So, if I send:

<presence><show>Away</show></presence>

to indicate I'm away, then org.jivesoftware.openfire.SessionManager#sessionUnavailable is called - which makes sense.

However, I do not believe that dropping the route to the client is the correct behaviour. There is still a route to the client. The "removeClientRoute" method is called when the session is actually removed.

It's possible the existing behaviour of re-adding the route to the client is there to cover-up a bug somewhere where routes to clients are lost, and re-adding them ensures that the route still exists.

[guusdk]

org.xmpp.packet.Presence#isAvailable() == true if there is a no type attribute of the presence packet after logging in.

So, org.jivesoftware.openfire.SessionManager#sessionUnavailable is called if a presence packet is received without a type attribute after more recently receiving one with.

I don't think that's right: session*Un*available() is called if a presence packet is received that is not available - in other words, when there is a type being defined.

Note that there are various types of presence (subscribe, subscribed etc.) for which we'd still not want the session to be marked unavailable, but those cases do not seem to be evaluated in the call flow.

The code is invoked in just one place: org.jivesoftware.openfire.session.LocalClientSession#setPresence

/**
 * Set the presence of this session
 *
 * @param presence The presence for the session
 */
@Override
public void setPresence(Presence presence) {
    Presence oldPresence = this.presence;
    this.presence = presence;
    if (oldPresence.isAvailable() && !this.presence.isAvailable()) {
        // The client is no longer available
        sessionManager.sessionUnavailable(this);
        // Mark that the session is no longer initialized. This means that if the user sends
        // an available presence again the session will be initialized again thus receiving
        // offline messages and offline presence subscription requests
        setInitialized(false);
        // Notify listeners that the session is no longer available
        PresenceEventDispatcher.unavailableSession(this, presence);
    }

Notice how, when sessionManager.sessionUnavailable is invoked, the session is unconditionally discarded.

[GregDThomas]

Yeah, quite right, my bad, got my negatives all muddle up. However, I still have two concerns;

1. By my reading, org.jivesoftware.openfire.session.LocalClientSession#setPresence will call sessionUnavailable on receipt of /any/ packet with a a type atttribute - not just type="unavailable". So a type="probe" will set the sessionUnavailable and drop the route?
   (assuming previous presence did not have a type)

2. Can the type="unavailable" be sent at any other time, or only when ending the session? https://xmpp.org/rfcs/rfc3921.html#rfc.section.5.1.4 suggests the former (I'm unavailable because I'm currently on the phone - <presence type="unavailable"><show>On the phone</show></presence>) - in which case the route should not be dropped. But that could be my bad reading.

Overall, I think the route should only be dropped when the session ends - removeSession(...) But it's quite possible that my understanding of what the "route" is. I'm currently assuming it's how Openfire knows how to communicate with a specific full JID, but that could be wrong, too.

[guusdk]

If I understand your argument correctly, then you're worried that this change might end up prematurely making session-related state switch to something that resembles 'offline' or 'ended' or 'unavailable' - something along those lines. I think we need not worry about that, given the code that also executes when this method is called.

The implementation of org.jivesoftware.openfire.session.LocalClientSession#setPresence (I've provided the implementation above) does three things:

1. Call sessionManager.sessionUnavailable(this) of which this PR suggests to change the inner workings.
2. Mark the session "no longer initialized" (by calling setInitialized(false))
3. Notify listeners that the session is no longer available.

Given the nature of 2 and 3, I think we can safely deduce the intended behavior for 1.

Thing 2 and 3 are unmodified by this PR. Their effects are pretty unambiguous: the session is made unavailable. Being "more careful" in the implementation of item 1 does not seem to be making much of a difference, as it's not called by other code.

[GregDThomas]

OK, I'll go with your suggestion ...