fix: client side disconnect incorrect client count on host-server sid…

…e [MTTB-135] (Up-Port) (#3075) * fix Fixed issue with the client count not being correct on the host or server side when a client disconnects itself from a session. * test validation tests for the changes/updates to this pr. * update updating the changelog.
Unity-Technologies · Sep 25, 2024 · ef9e1c1 · ef9e1c1
1 parent 3afc6f9
commit ef9e1c1
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 16 deletions.
diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md
@@ -12,6 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue with the client count not being correct on the host or server side when a client disconnects itself from a session. (#3075)
+
 ### Changed
 
 ## [2.0.0] - 2024-09-12

diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs
@@ -105,8 +105,12 @@ internal void InvokeOnClientConnectedCallback(ulong clientId)
                         continue;
                     }
 
-                    peerClientIds[idx] = peerId;
-                    ++idx;
+                    // This assures if the server has not timed out prior to the client synchronizing that it doesn't exceed the allocated peer count.
+                    if (peerClientIds.Length > idx)
+                    {
+                        peerClientIds[idx] = peerId;
+                        ++idx;
+                    }
                 }
 
                 try
@@ -496,24 +500,32 @@ internal void DisconnectEventHandler(ulong transportClientId)
             // Process the incoming message queue so that we get everything from the server disconnecting us or, if we are the server, so we got everything from that client.
             MessageManager.ProcessIncomingMessageQueue();
 
-            InvokeOnClientDisconnectCallback(clientId);
-
-            if (LocalClient.IsHost)
-            {
-                InvokeOnPeerDisconnectedCallback(clientId);
-            }
-
             if (LocalClient.IsServer)
             {
+                // We need to process the disconnection before notifying
                 OnClientDisconnectFromServer(clientId);
+
+                // Now notify the client has disconnected
+                InvokeOnClientDisconnectCallback(clientId);
+
+                if (LocalClient.IsHost)
+                {
+                    InvokeOnPeerDisconnectedCallback(clientId);
+                }
             }
-            else // As long as we are not in the middle of a shutdown
-            if (!NetworkManager.ShutdownInProgress)
+            else
             {
-                // We must pass true here and not process any sends messages as we are no longer connected.
-                // Otherwise, attempting to process messages here can cause an exception within UnityTransport
-                // as the client ID is no longer valid.
-                NetworkManager.Shutdown(true);
+                // Notify local client of disconnection
+                InvokeOnClientDisconnectCallback(clientId);
+
+                // As long as we are not in the middle of a shutdown
+                if (!NetworkManager.ShutdownInProgress)
+                {
+                    // We must pass true here and not process any sends messages as we are no longer connected.
+                    // Otherwise, attempting to process messages here can cause an exception within UnityTransport
+                    // as the client ID is no longer valid.
+                    NetworkManager.Shutdown(true);
+                }
             }
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
             s_TransportDisconnect.End();

diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs
@@ -82,7 +82,7 @@ protected override IEnumerator OnStartedServerAndClients()
         public IEnumerator ValidateApprovalTimeout()
         {
             // Just delay for a second
-            yield return new WaitForSeconds(1);
+            yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.25f);
 
             // Verify we haven't received the time out message yet
             NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage);

diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs
@@ -180,6 +180,9 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client
                 Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!");
                 Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ClientNetworkManagers[0]), $"Could not find the client {nameof(NetworkManager)} disconnect event entry!");
                 Assert.IsTrue(m_DisconnectedEvent[m_ClientNetworkManagers[0]].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the client {nameof(NetworkManager)} disconnect event entry!");
+                Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Count == 1, $"Expected connected client identifiers count to be 1 but it was {m_ServerNetworkManager.ConnectedClientsIds.Count}!");
+                Assert.IsTrue(m_ServerNetworkManager.ConnectedClients.Count == 1, $"Expected connected client identifiers count to be 1 but it was {m_ServerNetworkManager.ConnectedClients.Count}!");
+                Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsList.Count == 1, $"Expected connected client identifiers count to be 1 but it was {m_ServerNetworkManager.ConnectedClientsList.Count}!");
             }
 
             if (m_OwnerPersistence == OwnerPersistence.DestroyWithOwner)