fix: decoupling SessionManager from NetworkManager [MTT-2603] (#581)

* Decoupled SessionManager from NetworkManager

* simplified flow of clearing disconnected players data and reinitializing the rest on session end

* Added HasSessionStarted field to decide if we keep or delete a disconnecting player's data

* Renamed Clear to OnServerEnded and added call to it when host disconnects

Author: LPLafontaineB

Assets/BossRoom/Scripts/Server/Game/State/ServerBossRoomState.cs

@@ -61,6 +61,8 @@ public override void OnNetworkSpawn()
                 NetworkManager.SceneManager.OnSceneEvent += OnClientSceneChanged;
 
                 DoInitialSpawnIfPossible();
+
+                SessionManager<SessionPlayerData>.Instance.OnSessionStarted();
             }
         }
 

Assets/BossRoom/Scripts/Server/Game/State/ServerCharSelectState.cs

@@ -204,8 +204,6 @@ public override void OnNetworkSpawn()
                 CharSelectData.OnClientChangedSeat += OnClientChangedSeat;
 
                 NetworkManager.Singleton.SceneManager.OnSceneEvent += OnSceneEvent;
-
-                SessionManager<SessionPlayerData>.Instance.OnSessionStarted();
             }
         }
 

Assets/BossRoom/Scripts/Shared/Net/ConnectionManagement/GameNetPortal.cs

@@ -239,10 +239,10 @@ public void RequestDisconnect()
             if (NetManager.IsServer)
             {
                 NetManager.SceneManager.OnSceneEvent -= OnSceneEvent;
+                SessionManager<SessionPlayerData>.Instance.OnServerEnded();
             }
             m_ClientPortal.OnUserDisconnectRequest();
             m_ServerPortal.OnUserDisconnectRequest();
-            SessionManager<SessionPlayerData>.Instance.OnUserDisconnectRequest();
             NetManager.Shutdown();
         }
 

Assets/BossRoom/Scripts/Shared/Net/ConnectionManagement/ServerGameNetPortal.cs

@@ -102,6 +102,7 @@ void OnClientDisconnect(ulong clientId)
                 {
                     m_LobbyServiceFacade.DeleteLobbyAsync(m_LobbyServiceFacade.CurrentUnityLobby.Id);
                 }
+                SessionManager<SessionPlayerData>.Instance.OnServerEnded();
             }
             else
             {
@@ -112,6 +113,7 @@ void OnClientDisconnect(ulong clientId)
                     {
                         m_LobbyServiceFacade.RemovePlayerFromLobbyAsync(playerId, m_LobbyServiceFacade.CurrentUnityLobby.Id, null, null);
                     }
+                    SessionManager<SessionPlayerData>.Instance.DisconnectClient(clientId);
                 }
             }
         }

Packages/com.unity.multiplayer.samples.coop/Utilities/Net/SessionManager.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using Unity.Netcode;
 using UnityEngine;
 
 namespace Unity.Multiplayer.Samples.BossRoom
@@ -13,40 +12,24 @@ public interface ISessionPlayerData
     }
 
     /// <summary>
-    /// This class uses a GUID to bind a player to a session. Once that player connects to a host, the host associates
-    /// the current ClientID to the player's session GUID. If the player disconnects and reconnects to the same host,
-    /// the session is preserved.
+    /// This class uses a unique player ID to bind a player to a session. Once that player connects to a host, the host
+    /// associates the current ClientID to the player's unique ID. If the player disconnects and reconnects to the same
+    /// host, the session is preserved.
     /// </summary>
     /// <remarks>
-    /// Using a client-generated GUID and sending it directly could be problematic, as a malicious user could intercept
-    /// it and reuse it to impersonate the original user. We are currently investigating this to offer a solution that
-    /// handles security better.
+    /// Using a client-generated player ID and sending it directly could be problematic, as a malicious user could
+    /// intercept it and reuse it to impersonate the original user. We are currently investigating this to offer a
+    /// solution that handles security better.
     /// </remarks>
     /// <typeparam name="T"></typeparam>
     public class SessionManager<T> where T : struct, ISessionPlayerData
     {
-        NetworkManager m_NetworkManager;
-
         SessionManager()
         {
-            m_NetworkManager = NetworkManager.Singleton;
-            if (m_NetworkManager)
-            {
-                m_NetworkManager.OnServerStarted += ServerStartedHandler;
-            }
-
             m_ClientData = new Dictionary<string, T>();
             m_ClientIDToPlayerId = new Dictionary<ulong, string>();
         }
 
-        ~SessionManager()
-        {
-            if (m_NetworkManager)
-            {
-                m_NetworkManager.OnServerStarted -= ServerStartedHandler;
-            }
-        }
-
         public static SessionManager<T> Instance => s_Instance ??= new SessionManager<T>();
 
         static SessionManager<T> s_Instance;
@@ -61,46 +44,45 @@ public class SessionManager<T> where T : struct, ISessionPlayerData
         /// </summary>
         Dictionary<ulong, string> m_ClientIDToPlayerId;
 
+        bool m_HasSessionStarted;
+
         /// <summary>
-        /// Handles the case where NetworkManager has told us a client has disconnected. This includes ourselves, if we're the host,
-        /// and the server is stopped."
+        /// Handles client disconnect."
         /// </summary>
-        void OnClientDisconnect(ulong clientId)
+        public void DisconnectClient(ulong clientId)
         {
-            if (m_ClientIDToPlayerId.TryGetValue(clientId, out var playerId))
+            if (m_HasSessionStarted)
             {
-                if (GetPlayerData(playerId)?.ClientID == clientId)
+                // Mark client as disconnected, but keep their data so they can reconnect.
+                if (m_ClientIDToPlayerId.TryGetValue(clientId, out var playerId))
                 {
-                    var clientData = m_ClientData[playerId];
-                    clientData.IsConnected = false;
-                    m_ClientData[playerId] = clientData;
+                    if (GetPlayerData(playerId)?.ClientID == clientId)
+                    {
+                        var clientData = m_ClientData[playerId];
+                        clientData.IsConnected = false;
+                        m_ClientData[playerId] = clientData;
+                    }
                 }
             }
-
-            if (clientId == m_NetworkManager.LocalClientId)
+            else
             {
-                //the SessionManager may be initialized again, which will cause its OnNetworkSpawn to be called again.
-                //Consequently we need to unregister anything we registered, when the NetworkManager is shutting down.
-                m_NetworkManager.OnClientDisconnectCallback -= OnClientDisconnect;
+                // Session has not started, no need to keep their data
+                if (m_ClientIDToPlayerId.TryGetValue(clientId, out var playerId))
+                {
+                    m_ClientIDToPlayerId.Remove(clientId);
+                    if (GetPlayerData(playerId)?.ClientID == clientId)
+                    {
+                        m_ClientData.Remove(playerId);
+                    }
+                }
             }
         }
 
         /// <summary>
-        /// Handles the flow when a user has requested a disconnect via UI (which can be invoked on the Host, and thus must be
-        /// handled in server code).
+        ///
         /// </summary>
-        public void OnUserDisconnectRequest()
-        {
-            Clear();
-        }
-
-        void Clear()
-        {
-            //resets all our runtime state.
-            m_ClientData.Clear();
-            m_ClientIDToPlayerId.Clear();
-        }
-
+        /// <param name="playerId">This is the playerId that is unique to this client and persists across multiple logins from the same client</param>
+        /// <returns>True if a player with this ID is already connected.</returns>
         public bool IsDuplicateConnection(string playerId)
         {
             return m_ClientData.ContainsKey(playerId) && m_ClientData[playerId].IsConnected;
@@ -148,14 +130,19 @@ public void SetupConnectingPlayerSessionData(ulong clientId, string playerId, T
             m_ClientData[playerId] = sessionPlayerData;
         }
 
+        /// <summary>
+        ///
+        /// </summary>
+        /// <param name="clientId"> id of the client whose data is requested</param>
+        /// <returns>The Player ID matching the given client ID</returns>
         public string GetPlayerId(ulong clientId)
         {
             if (m_ClientIDToPlayerId.TryGetValue(clientId, out string playerId))
             {
                 return playerId;
             }
 
-            Debug.LogError($"No client guid found mapped to the given client ID: {clientId}");
+            Debug.LogError($"No client player ID found mapped to the given client ID: {clientId}");
             return null;
         }
 
@@ -211,20 +198,11 @@ public void SetPlayerData(ulong clientId, T sessionPlayerData)
         }
 
         /// <summary>
-        /// Called after the server is created-  This is primarily meant for the host server to clean up or handle/set state as its starting up
+        /// Marks the current session as started, so from now on we keep the data of disconnected players.
         /// </summary>
-        void ServerStartedHandler()
-        {
-            if (m_NetworkManager.IsServer)
-            {
-                //O__O if adding any event registrations here, please add an unregistration in OnClientDisconnect.
-                m_NetworkManager.OnClientDisconnectCallback += OnClientDisconnect;
-            }
-        }
-
         public void OnSessionStarted()
         {
-            ClearDisconnectedPlayersData();
+            m_HasSessionStarted = true;
         }
 
         /// <summary>
@@ -233,36 +211,41 @@ public void OnSessionStarted()
         public void OnSessionEnded()
         {
             ClearDisconnectedPlayersData();
-            List<ulong> connectedClientIds = new List<ulong>(m_NetworkManager.ConnectedClientsIds);
+            ReinitializePlayersData();
+            m_HasSessionStarted = false;
+        }
+
+        /// <summary>
+        /// Resets all our runtime state, so it is ready to be reinitialized when starting a new server
+        /// </summary>
+        public void OnServerEnded()
+        {
+            m_ClientData.Clear();
+            m_ClientIDToPlayerId.Clear();
+            m_HasSessionStarted = false;
+        }
+
+        void ReinitializePlayersData()
+        {
             foreach (var id in m_ClientIDToPlayerId.Keys)
             {
-                if (connectedClientIds.Contains(id))
-                {
-                    string playerId = m_ClientIDToPlayerId[id];
-                    T sessionPlayerData = m_ClientData[playerId];
-                    sessionPlayerData.Reinitialize();
-                    m_ClientData[playerId] = sessionPlayerData;
-                }
+                string playerId = m_ClientIDToPlayerId[id];
+                T sessionPlayerData = m_ClientData[playerId];
+                sessionPlayerData.Reinitialize();
+                m_ClientData[playerId] = sessionPlayerData;
             }
         }
 
         void ClearDisconnectedPlayersData()
         {
             List<ulong> idsToClear = new List<ulong>();
-            List<ulong> connectedClientIds = new List<ulong>(m_NetworkManager.ConnectedClientsIds);
             foreach (var id in m_ClientIDToPlayerId.Keys)
             {
-                if (!connectedClientIds.Contains(id))
+                var data = GetPlayerData(id);
+                if (data is {IsConnected: false})
                 {
                     idsToClear.Add(id);
                 }
-                else
-                {
-                    string playerId = m_ClientIDToPlayerId[id];
-                    T sessionPlayerData = m_ClientData[playerId];
-                    sessionPlayerData.Reinitialize();
-                    m_ClientData[playerId] = sessionPlayerData;
-                }
             }
 
             foreach (var id in idsToClear)