fix: client side disconnect incorrect client count on host-server side [MTTB-135] (#2941)

* fix

This resolves the issue with the client count not being correct on the host or server side when a client disconnects itself from a session.

This also resolves the issue with the host trying to send itself a message that it has connected when first starting up.

Fixing issue with NetworkSceneManager finishing synchronization prior to the server connection timing out.

* test

Adding validation for the connected clients count on the server side when the client-side disconnects.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -15,6 +15,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. (#2941)
+- Fixed issue with the host trying to send itself a message that it has connected when first starting up. (#2941)
 - Fixed issue where in-scene placed NetworkObjects could be destroyed if a client disconnects early and/or before approval. (#2923)
 - Fixed issue where `NetworkDeltaPosition` would "jitter" periodically if both unreliable delta state updates and half-floats were used together. (#2922)
 - Fixed issue where `NetworkRigidbody2D` would not properly change body type based on the instance's authority when spawned. (#2916)

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

@@ -104,9 +104,11 @@ internal void InvokeOnClientConnectedCallback(ulong clientId)
                     {
                         continue;
                     }
-
-                    peerClientIds[idx] = peerId;
-                    ++idx;
+                    if (peerClientIds.Length > idx)
+                    {
+                        peerClientIds[idx] = peerId;
+                        ++idx;
+                    }
                 }
 
                 try
@@ -491,24 +493,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 (NetworkManager.IsServer)
@@ -900,9 +910,14 @@ internal NetworkClient AddClient(ulong clientId)
 
             ConnectedClients.Add(clientId, networkClient);
             ConnectedClientsList.Add(networkClient);
-            var message = new ClientConnectedMessage { ClientId = clientId };
-            NetworkManager.MessageManager.SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, ConnectedClientIds);
             ConnectedClientIds.Add(clientId);
+            // Host should not send this message to itself
+            if (clientId != NetworkManager.ServerClientId)
+            {
+                var message = new ClientConnectedMessage { ClientId = clientId };
+                NetworkManager.MessageManager.SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, ConnectedClientIds);
+            }
+
             return networkClient;
         }
 

com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs

@@ -81,7 +81,7 @@ protected override IEnumerator OnStartedServerAndClients()
         public IEnumerator ValidateApprovalTimeout()
         {
             // Delay for half of the wait period
-            yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.5f);
+            yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.25f);
 
             // Verify we haven't received the time out message yet
             NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage);

com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs

@@ -182,6 +182,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(clientManager), $"Could not find the client {nameof(NetworkManager)} disconnect event entry!");
                 Assert.IsTrue(m_DisconnectedEvent[clientManager].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)