Mixedrealitytoolkit-unity: Sessions list not properly updated in ServerSessionsTracker

[edwinckc]

In ServerSessionsTracker.cs, there is an OnSessionClosed() event handler. It takes the _session_ param and removes it from the local _Sessions_ list. However, the session actually doesn't get removed from the list. Adding debug messages shows that Sessions.Contains(session) returns false, and Sessions.Remove(session) also returns false. Sessions.Count remains the same both before and after the Remove() method is called.

The session passed as a param has the same name and hash code as an existing session inside Sessions. I'm not sure how equality is checked here, but perhaps it's comparing by reference and a new copy was made somewhere along the way.

One problem I've encountered with this bug is using the AutoJoinSession.cs script. It checks whether the local Sessions list contains a session with the same name as the one we want to join. That means if I want to connect to the same session again, it will find the local session inside Sessions, and try to connect to that. The server doesn't receive anything, and will not send back any events.

To reproduce:

1. Attach an AutoJoinSession script to join a session.
2. Call SessionManager.GetCurrentSession().Leave(). You will successfully leave the server, but the local Sessions list is not updated.
3. Attach a new AutoJoinSession script to join a session of the same name, and it will not connect.

[StephenHodgson]

Thanks for the report.

[edwinckc]

Hey @StephenHodgson, just wondering if this was actually fixed in #742?

I was still getting the same error, even after I pulled your HTK-WorldAnchorManagerUpdate branch. Locally, _ServerSessionsTracker_ still fails to remove the closed session. Upon attempting to reconnect, it tries to connect to the locally store Session object and results in handshake error code 1. I am using a modified version of your new _AutoJoinSessionAndRoom_ script, which works very well btw. I let the _SharingStage_ handle the server connection, and enable _AutoJoinSessionAndRoom_ to connect to session and room. When disconnecting, I disable the _AutoJoinSessionAndRoom_, before calling SharingStage.Instance.SessionsTracker.LeaveCurrentSession().

Original code:

private void OnSessionClosed(Session session)
{
    SessionClosed.RaiseEvent(session);
    Sessions.Remove(session);
}

Fix (assumes that session names must be unique):

private void OnSessionClosed(Session session)
{
    int originalCount = Sessions.Count;

    SessionClosed.RaiseEvent(session);
    Sessions.Remove(session);

    if(Sessions.Count >= originalCount)
    {
        Debug.LogErrorFormat("[ServerSessionsTracker] Started with {0} session(s) and still have {1} session(s) in the list.", originalCount, Sessions.Count);
        Debug.Log("[ServerSessionsTracker] Try to remove session manually...");
        Session sessionToRemove = Sessions.Where(x => x.GetName().ToString() == session.GetName().ToString()).FirstOrDefault();
        if(sessionToRemove != null)
            Sessions.Remove(sessionToRemove);

        Debug.LogFormat("...{0} session(s) in the list after manual removal.", Sessions.Count);
    }
}

Besides this, the updated branch seems much more robust than before. I am able to quickly connect, disconnect, and reconnect, without any problems with network state or events not being fired. I remember getting handshake errors, null references, errors that were logged but not handled from server side, and something about MachineState (which crashed the app). Much thanks for all your work on this.

[StephenHodgson]

Hmm this might be a bug in the main sharing service SDK that comes from the MixedRealityToolkit base.

[StephenHodgson]

Are you still getting that hand shake error with your modified version?

[StephenHodgson]

Or does your suggestion fix the issue you had said you're having?

[edwinckc]

Yup, it fixes the handshake error. It essentially ensures that there are no local copies of sessions which no longer exist on the server. Previously, these extra local sessions would have no matching endpoint on the server, and result in a handshake error. (my guess)

I should add that I am using Adhoc sessions. If sessions on the server were always persistent, I imagine this problem would be much less common.

[StephenHodgson]

Can you submit this as a PR against that branch? I'd appreciate it.

Unless you don't mind me doing a straight copy. It's up to you.

[edwinckc]

Straight copy works for me!

[StephenHodgson]

Hmm, I'm unsure why this fixes your issue with the hand shake.

Essentially we're just calling Sessions.Remove(session); again if we didn't _actually_ remove it.

Seems like actually going through the list and getting the right session is better somehow.
The Session class must not have been written with good equality checking.

[edwinckc]

I found it really strange too, and maybe I didn't explain it too well in my original post. For whatever reason, the _session_ param passed into the event handler does not match any records in List Sessions. Checking for equality through the session name (instead of the actual session itself) gets around this strange mismatch, and allows the session to be removed properly.

[StephenHodgson]

I simplified it a bit more.

Instead we will go though our list and remove it after checking the name.

Great Catch!

        private void OnSessionClosed(Session session)
        {
            for (int i = 0; i < Sessions.Count; i++)
            {
                if (Sessions[i].GetName().ToString().Equals(session.GetName().ToString()))
                {
                    SessionClosed.RaiseEvent(session);
                    Sessions.Remove(session);
                }
            }
        }

[edwinckc]

Looks good to me, but something minor:

SessionClosed.RaiseEvent(session); // Probably okay with session or Sessions[i]
Sessions.Remove(session); // Should this remove Sessions[i] instead?

[StephenHodgson]

Sure. Fixed.

[StephenHodgson]

@edwinckc one last thing. Please be sure to review and approve that PR this change is supposed to fix.

Thanks.