fix: client not approved timeout exception

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -41,6 +41,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue with approval time out where upon a client not being approved the server-side time out coroutine, when it ended, could throw an exception on third party transports.(#2466)
+- Fixed issue where the approval time out coroutine was not being stopped once a client was approved or not approved. (#2466)
 - Fixed an issue where Named Message Handlers could remove themselves causing an exception when the metrics tried to access the name of the message.(#2426)
 - Fixed registry of public `NetworkVariable`s in derived `NetworkBehaviour`s (#2423)
 - Fixed issue where runtime association of `Animator` properties to `AnimationCurve`s would cause `NetworkAnimator` to attempt to update those changes. (#2416)

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -1615,7 +1615,12 @@ private IEnumerator ApprovalTimeout(ulong clientId)
 
                 if (IsServer)
                 {
-                    DisconnectClient(clientId);
+                    // Only disconnect when the connection times out.
+                    // If the connection is not approved it will automatically disconnect the client
+                    if (timedOut)
+                    {
+                        DisconnectClient(clientId);
+                    }
                 }
                 else
                 {
@@ -1634,6 +1639,52 @@ internal ulong ClientIdToTransportId(ulong clientId)
             return clientId == ServerClientId ? m_ServerTransportId : m_ClientIdToTransportIdMap[clientId];
         }
 
+        internal Dictionary<ulong, Coroutine> ApprovalTimeouts = new Dictionary<ulong, Coroutine>();
+
+        /// <summary>
+        /// Starts a timeout coroutine during the initial connection sequence.
+        /// </summary>
+        /// <remarks>
+        /// The clientId is relative to the local instance.
+        /// Server: Uses the client identifier to associate started coroutines.
+        /// Client: Uses the server identifier to associate the started coroutine.
+        /// </remarks>
+        /// <param name="clientId"></param>
+        private void StartClientTimeout(ulong clientId)
+        {
+            // Don't need to start a timeout coroutine locally for the host
+            if (IsServer && clientId == ServerClientId)
+            {
+                return;
+            }
+            if (!ApprovalTimeouts.ContainsKey(clientId))
+            {
+                ApprovalTimeouts.Add(clientId, StartCoroutine(ApprovalTimeout(clientId)));
+            }
+            else
+            {
+                // Theoretically this should never happen, but in case it does log a warning
+                if (LogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning($"Attempting to start a client approval timeout while one already exists!");
+                }
+                ApprovalTimeouts[clientId] = StartCoroutine(ApprovalTimeout(clientId));
+            }
+        }
+
+        /// <summary>
+        /// Stops the time out coroutine
+        /// </summary>
+        internal void StopClientTimeout(ulong clientId)
+        {
+            if (!ApprovalTimeouts.ContainsKey(clientId))
+            {
+                return;
+            }
+            StopCoroutine(ApprovalTimeouts[clientId]);
+            ApprovalTimeouts.Remove(clientId);
+        }
+
         private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, ArraySegment<byte> payload, float receiveTime)
         {
             var transportId = clientId;
@@ -1675,7 +1726,7 @@ private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, A
                             ConnectionState = PendingClient.State.PendingConnection
                         });
 
-                        StartCoroutine(ApprovalTimeout(clientId));
+                        StartClientTimeout(clientId);
                     }
                     else
                     {
@@ -1685,7 +1736,8 @@ private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, A
                         }
 
                         SendConnectionRequest();
-                        StartCoroutine(ApprovalTimeout(clientId));
+
+                        StartClientTimeout(clientId);
                     }
 
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
@@ -2046,6 +2098,8 @@ private void SyncTime()
         /// <param name="response">The response to allow the player in or not, with its parameters</param>
         internal void HandleConnectionApproval(ulong ownerClientId, ConnectionApprovalResponse response)
         {
+            // Stop the server-side coroutine whether approved or not
+            StopClientTimeout(ownerClientId);
             if (response.Approved)
             {
                 // Inform new client it got approved
@@ -2164,7 +2218,6 @@ internal void HandleConnectionApproval(ulong ownerClientId, ConnectionApprovalRe
 
                     MessagingSystem.ProcessSendQueues();
                 }
-
                 PendingClients.Remove(ownerClientId);
                 DisconnectRemoteClient(ownerClientId);
             }

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs

@@ -120,6 +120,10 @@ public void Handle(ref NetworkContext context)
             networkManager.LocalClient = new NetworkClient() { ClientId = networkManager.LocalClientId };
             networkManager.IsApproved = true;
 
+            // Upon receiving the approved message, stop the time out coroutine.
+            // (Client's associate their time out coroutine with the server identifier.)
+            networkManager.StopClientTimeout(NetworkManager.ServerClientId);
+
             // Only if scene management is disabled do we handle NetworkObject synchronization at this point
             if (!networkManager.NetworkConfig.EnableSceneManagement)
             {

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

@@ -9,14 +9,16 @@ namespace Unity.Netcode.RuntimeTests
 {
     [TestFixture(ApprovalTimedOutTypes.ServerDoesNotRespond)]
     [TestFixture(ApprovalTimedOutTypes.ClientDoesNotRequest)]
+    [TestFixture(ApprovalTimedOutTypes.ClientIsNotApproved)]
     public class ConnectionApprovalTimeoutTests : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 1;
 
         public enum ApprovalTimedOutTypes
         {
             ClientDoesNotRequest,
-            ServerDoesNotRespond
+            ServerDoesNotRespond,
+            ClientIsNotApproved,
         }
 
         private ApprovalTimedOutTypes m_ApprovalFailureType;
@@ -48,13 +50,35 @@ protected override IEnumerator OnTearDown()
 
         protected override void OnServerAndClientsCreated()
         {
-            m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
             m_ServerNetworkManager.LogLevel = LogLevel.Developer;
-            m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
+            m_ServerNetworkManager.NetworkConfig.ConnectionApproval = m_ApprovalFailureType == ApprovalTimedOutTypes.ClientIsNotApproved;
+            if (m_ApprovalFailureType == ApprovalTimedOutTypes.ClientIsNotApproved)
+            {
+                m_ServerNetworkManager.ConnectionApprovalCallback = NetworkManagerObject_ConnectionApprovalCallback;
+            }
+            else
+            {
+                m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
+                m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
+            }
+            m_ClientNetworkManagers[0].NetworkConfig.ConnectionApproval = m_ApprovalFailureType == ApprovalTimedOutTypes.ClientIsNotApproved;
             m_ClientNetworkManagers[0].LogLevel = LogLevel.Developer;
             base.OnServerAndClientsCreated();
         }
 
+        private bool m_ClientWasNotApproved;
+        private void NetworkManagerObject_ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest request, NetworkManager.ConnectionApprovalResponse response)
+        {
+            response.Approved = request.ClientNetworkId == NetworkManager.ServerClientId;
+            m_ClientWasNotApproved = !response.Approved;
+            if (!response.Approved)
+            {
+                Assert.IsTrue(m_ServerNetworkManager.ApprovalTimeouts.Count > 0, $"There are ({m_ServerNetworkManager.ApprovalTimeouts.Count}) approval timeout coroutines when there should at least be 1!");
+            }
+            response.Reason = "Testing approval failure and TimedOut coroutine.";
+            VerboseDebug($"Client {request.ClientNetworkId} was approved {response.Approved}");
+        }
+
         protected override IEnumerator OnStartedServerAndClients()
         {
             if (m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond)
@@ -64,7 +88,7 @@ protected override IEnumerator OnStartedServerAndClients()
                 m_ExpectedLogMessage = new Regex("Timed out waiting for the server to approve the connection request.");
                 m_LogType = LogType.Log;
             }
-            else
+            else if (m_ApprovalFailureType == ApprovalTimedOutTypes.ClientDoesNotRequest)
             {
                 // We catch (don't process) the incoming connection request message to simulate a transport connection but the client never
                 // sends (or takes too long to send) the connection request.
@@ -74,23 +98,37 @@ protected override IEnumerator OnStartedServerAndClients()
                 m_ExpectedLogMessage = new Regex("Server detected a transport connection from Client-1, but timed out waiting for the connection request message.");
                 m_LogType = LogType.Warning;
             }
+            // Otherwise the not approved test doesn't intercept any messages
             yield return null;
         }
 
         [UnityTest]
         public IEnumerator ValidateApprovalTimeout()
         {
-            // Delay for half of the wait period
-            yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.5f);
+            if (m_ApprovalFailureType != ApprovalTimedOutTypes.ClientIsNotApproved)
+            {
+                // Delay for half of the wait period
+                yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.5f);
+
+                // Verify we haven't received the time out message yet
+                NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage);
 
-            // Verify we haven't received the time out message yet
-            NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage);
+                // Wait for 3/4s of the time out period to pass (totaling 1.25x the wait period)
+                yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.75f);
 
-            // Wait for 3/4s of the time out period to pass (totaling 1.25x the wait period)
-            yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.75f);
+                // We should have the test relative log message by this time.
+                NetcodeLogAssert.LogWasReceived(m_LogType, m_ExpectedLogMessage);
+            }
+            else
+            {
+                // Make sure a client was not approved
+                yield return WaitForConditionOrTimeOut(() => m_ClientWasNotApproved);
+                AssertOnTimeout($"Did not detect that the client was not approved while waiting!");
 
-            // We should have the test relative log message by this time.
-            NetcodeLogAssert.LogWasReceived(m_LogType, m_ExpectedLogMessage);
+                // Make sure that there are no timed out coroutines left running on the server after a client is not approved
+                yield return WaitForConditionOrTimeOut(() => m_ServerNetworkManager.ApprovalTimeouts.Count == 0);
+                AssertOnTimeout($"Waited for {nameof(NetworkManager.ApprovalTimeouts)} to reach a count of 0 but is still at {m_ServerNetworkManager.ApprovalTimeouts.Count}!");
+            }
 
             // It should only have the host client connected
             Assert.AreEqual(1, m_ServerNetworkManager.ConnectedClients.Count, $"Expected only one client when there were {m_ServerNetworkManager.ConnectedClients.Count} clients connected!");