fix: NetworkManager ApprovalTimeout should not depend upon client synchronization (#2261)

* fix
This fix separates the IsConnectedClient from the approval process by adding an IsApproved flag.
This also has a fix to prevent the domain backup error during serialization.

* test
Added the ability to bypass the entire NetcodeIntegrationTest connection approval process after the server and clients have been started.  This allows us to now use NetcodeIntegrationTest for unique connection oriented tests without being bound to the asserts if not all clients connected properly.
Refactored for ConnectionApprovalTimeoutTests to use the added m_BypassConnectionTimeout to bypass the waiting for clients to connect. It still uses the message hook catch technique to simulate the timeout scenarios where either a server detects a transport connection but never receives a connection request or a client sends the connection request but never receives approval for the connection.

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -7,6 +7,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com).
 
+## [Unreleased]
+
+### Added
+- Added `NetworkManager.IsApproved` flag that is set to `true` a client has been approved.(#2261)
+
+### Changed
+
+
+### Fixed
+
+- Fixed `NetworkManager.ApprovalTimeout` will not timeout due to slower client synchronization times as it now uses the added `NetworkManager.IsApproved` flag to determined if the client has been approved or not.(#2261)
+
+
 ## [1.1.0] - 2022-10-19
 
 ### Added

com.unity.netcode.gameobjects/Components/NetworkAnimator.cs

@@ -232,6 +232,10 @@ namespace Unity.Netcode.Components
         private void BuildTransitionStateInfoList()
         {
 #if UNITY_EDITOR
+            if (UnityEditor.EditorApplication.isUpdating)
+            {
+                return;
+            }
             TransitionStateInfoList = new List<TransitionStateinfo>();
             var animatorController = m_Animator.runtimeAnimatorController as AnimatorController;
             if (animatorController == null)

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

@@ -366,10 +366,22 @@ namespace Unity.Netcode
         public bool IsListening { get; internal set; }
 
         /// <summary>
-        /// Gets if we are connected as a client
+        /// When true, the client is connected, approved, and synchronized with
+        /// the server.
         /// </summary>
         public bool IsConnectedClient { get; internal set; }
 
+        /// <summary>
+        /// Is true when the client has been approved.
+        /// </summary>
+        /// <remarks>
+        /// This only reflects the client's approved status and does not mean the client
+        /// has finished the connection and synchronization process. The server-host will
+        /// always be approved upon being starting the <see cref="NetworkManager"/>
+        /// <see cref="IsConnectedClient"/>
+        /// </remarks>
+        public bool IsApproved { get; internal set; }
+
         /// <summary>
         /// Can be used to determine if the <see cref="NetworkManager"/> is currently shutting itself down
         /// </summary>
@@ -877,6 +889,8 @@ namespace Unity.Netcode
                 return;
             }
 
+            IsApproved = false;
+
             ComponentFactory.SetDefaults();
 
             if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
@@ -1018,7 +1032,6 @@ namespace Unity.Netcode
             }
 
             Initialize(true);
-
             IsServer = true;
             IsClient = false;
             IsListening = true;
@@ -1031,6 +1044,7 @@ namespace Unity.Netcode
                     SpawnManager.ServerSpawnSceneObjectsOnStartSweep();
 
                     OnServerStarted?.Invoke();
+                    IsApproved = true;
                     return true;
                 }
                 else
@@ -1152,6 +1166,7 @@ namespace Unity.Netcode
                 }
 
                 response.Approved = true;
+                IsApproved = true;
                 HandleConnectionApproval(ServerClientId, response);
             }
             else
@@ -1413,6 +1428,7 @@ namespace Unity.Netcode
             }
 
             IsConnectedClient = false;
+            IsApproved = false;
 
             // We need to clean up NetworkObjects before we reset the IsServer
             // and IsClient properties. This provides consistency of these two
@@ -1649,59 +1665,71 @@ namespace Unity.Netcode
 
         private IEnumerator ApprovalTimeout(ulong clientId)
         {
-            if (IsServer)
+            var timeStarted = IsServer ? LocalTime.TimeAsFloat : Time.realtimeSinceStartup;
+            var timedOut = false;
+            var connectionApproved = false;
+            var connectionNotApproved = false;
+            var timeoutMarker = timeStarted + NetworkConfig.ClientConnectionBufferTimeout;
+
+            while (IsListening && !ShutdownInProgress && !timedOut && !connectionApproved)
             {
-                NetworkTime timeStarted = LocalTime;
+                yield return null;
+                // Check if we timed out
+                timedOut = timeoutMarker < (IsServer ? LocalTime.TimeAsFloat : Time.realtimeSinceStartup);
 
-                //We yield every frame incase a pending client disconnects and someone else gets its connection id
-                while (IsListening && (LocalTime - timeStarted).Time < NetworkConfig.ClientConnectionBufferTimeout && PendingClients.ContainsKey(clientId))
+                if (IsServer)
                 {
-                    yield return null;
+                    // When the client is no longer in the pending clients list and is in the connected clients list
+                    // it has been approved
+                    connectionApproved = !PendingClients.ContainsKey(clientId) && ConnectedClients.ContainsKey(clientId);
+
+                    // For the server side, if the client is in neither list then it was declined or the client disconnected
+                    connectionNotApproved = !PendingClients.ContainsKey(clientId) && !ConnectedClients.ContainsKey(clientId);
                 }
-
-                if (!IsListening)
+                else
                 {
-                    yield break;
-                }
-
-                if (PendingClients.ContainsKey(clientId) && !ConnectedClients.ContainsKey(clientId))
-                {
-                    // Timeout
-                    if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
-                    {
-                        NetworkLog.LogInfo($"Client {clientId} Handshake Timed Out");
-                    }
-
-                    DisconnectClient(clientId);
+                    connectionApproved = IsApproved;
                 }
             }
-            else
+
+            // Exit coroutine if we are no longer listening or a shutdown is in progress (client or server)
+            if (!IsListening || ShutdownInProgress)
             {
-                float timeStarted = Time.realtimeSinceStartup;
-                //We yield every frame in case a pending client disconnects and someone else gets its connection id
-                while (IsListening && (Time.realtimeSinceStartup - timeStarted) < NetworkConfig.ClientConnectionBufferTimeout && !IsConnectedClient)
-                {
-                    yield return null;
-                }
+                yield break;
+            }
 
-                if (!IsConnectedClient && NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+            // If the client timed out or was not approved
+            if (timedOut || connectionNotApproved)
+            {
+                // Timeout
+                if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
                 {
-                    // TODO: Possibly add a callback for users to be notified of this condition here?
-                    NetworkLog.LogWarning($"[ApprovalTimeout] Client timed out! You might need to increase the {nameof(NetworkConfig.ClientConnectionBufferTimeout)} duration.  Approval Check Start: {timeStarted} | Approval Check Stopped: {Time.realtimeSinceStartup}");
-                }
-
-                if (!IsListening)
-                {
-                    yield break;
-                }
-
-                if (!IsConnectedClient)
-                {
-                    // Timeout
-                    if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
+                    if (timedOut)
                     {
-                        NetworkLog.LogInfo("Server Handshake Timed Out");
+                        if (IsServer)
+                        {
+                            // Log a warning that the transport detected a connection but then did not receive a follow up connection request message.
+                            // (hacking or something happened to the server's network connection)
+                            NetworkLog.LogWarning($"Server detected a transport connection from Client-{clientId}, but timed out waiting for the connection request message.");
+                        }
+                        else
+                        {
+                            // We only provide informational logging for the client side
+                            NetworkLog.LogInfo("Timed out waiting for the server to approve the connection request.");
+                        }
                     }
+                    else if (connectionNotApproved)
+                    {
+                        NetworkLog.LogInfo($"Client-{clientId} was either denied approval or disconnected while being approved.");
+                    }
+                }
+
+                if (IsServer)
+                {
+                    DisconnectClient(clientId);
+                }
+                else
+                {
                     Shutdown(true);
                 }
             }

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

@@ -79,6 +79,7 @@ namespace Unity.Netcode
             networkManager.NetworkTickSystem.Reset(networkManager.NetworkTimeSystem.LocalTime, networkManager.NetworkTimeSystem.ServerTime);
 
             networkManager.LocalClient = new NetworkClient() { ClientId = networkManager.LocalClientId };
+            networkManager.IsApproved = true;
 
             // Only if scene management is disabled do we handle NetworkObject synchronization at this point
             if (!networkManager.NetworkConfig.EnableSceneManagement)

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

@@ -131,6 +131,18 @@ namespace Unity.Netcode.TestHelpers.Runtime
 
         protected bool m_EnableVerboseDebug { get; set; }
 
+        /// <summary>
+        /// When set to true, this will bypass the entire
+        /// wait for clients to connect process.
+        /// </summary>
+        /// <remarks>
+        /// CAUTION:
+        /// Setting this to true will bypass other helper
+        /// identification related code, so this should only
+        /// be used for connection failure oriented testing
+        /// </remarks>
+        protected bool m_BypassConnectionTimeout { get; set; }
+
         /// <summary>
         /// Used to display the various integration test
         /// stages and can be used to log verbose information
@@ -455,31 +467,36 @@ namespace Unity.Netcode.TestHelpers.Runtime
                 // Notification that the server and clients have been started
                 yield return OnStartedServerAndClients();
 
-                // Wait for all clients to connect
-                yield return WaitForClientsConnectedOrTimeOut();
-                AssertOnTimeout($"{nameof(StartServerAndClients)} timed out waiting for all clients to be connected!");
-
-                if (m_UseHost || m_ServerNetworkManager.IsHost)
+                // When true, we skip everything else (most likely a connection oriented test)
+                if (!m_BypassConnectionTimeout)
                 {
-                    // Add the server player instance to all m_ClientSidePlayerNetworkObjects entries
-                    var serverPlayerClones = Object.FindObjectsOfType<NetworkObject>().Where((c) => c.IsPlayerObject && c.OwnerClientId == m_ServerNetworkManager.LocalClientId);
-                    foreach (var playerNetworkObject in serverPlayerClones)
+                    // Wait for all clients to connect
+                    yield return WaitForClientsConnectedOrTimeOut();
+
+                    AssertOnTimeout($"{nameof(StartServerAndClients)} timed out waiting for all clients to be connected!");
+
+                    if (m_UseHost || m_ServerNetworkManager.IsHost)
                     {
-                        if (!m_PlayerNetworkObjects.ContainsKey(playerNetworkObject.NetworkManager.LocalClientId))
+                        // Add the server player instance to all m_ClientSidePlayerNetworkObjects entries
+                        var serverPlayerClones = Object.FindObjectsOfType<NetworkObject>().Where((c) => c.IsPlayerObject && c.OwnerClientId == m_ServerNetworkManager.LocalClientId);
+                        foreach (var playerNetworkObject in serverPlayerClones)
                         {
-                            m_PlayerNetworkObjects.Add(playerNetworkObject.NetworkManager.LocalClientId, new Dictionary<ulong, NetworkObject>());
+                            if (!m_PlayerNetworkObjects.ContainsKey(playerNetworkObject.NetworkManager.LocalClientId))
+                            {
+                                m_PlayerNetworkObjects.Add(playerNetworkObject.NetworkManager.LocalClientId, new Dictionary<ulong, NetworkObject>());
+                            }
+                            m_PlayerNetworkObjects[playerNetworkObject.NetworkManager.LocalClientId].Add(m_ServerNetworkManager.LocalClientId, playerNetworkObject);
                         }
-                        m_PlayerNetworkObjects[playerNetworkObject.NetworkManager.LocalClientId].Add(m_ServerNetworkManager.LocalClientId, playerNetworkObject);
                     }
+
+                    ClientNetworkManagerPostStartInit();
+
+                    // Notification that at this time the server and client(s) are instantiated,
+                    // started, and connected on both sides.
+                    yield return OnServerAndClientsConnected();
+
+                    VerboseDebug($"Exiting {nameof(StartServerAndClients)}");
                 }
-
-                ClientNetworkManagerPostStartInit();
-
-                // Notification that at this time the server and client(s) are instantiated,
-                // started, and connected on both sides.
-                yield return OnServerAndClientsConnected();
-
-                VerboseDebug($"Exiting {nameof(StartServerAndClients)}");
             }
         }
 

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

@@ -1,5 +1,4 @@
 using System.Collections;
-using System.Linq;
 using System.Text.RegularExpressions;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
@@ -8,97 +7,95 @@ using UnityEngine.TestTools;
 
 namespace Unity.Netcode.RuntimeTests
 {
-    [TestFixture(true)]
-    [TestFixture(false)]
+    [TestFixture(ApprovalTimedOutTypes.ServerDoesNotRespond)]
+    [TestFixture(ApprovalTimedOutTypes.ClientDoesNotRequest)]
     public class ConnectionApprovalTimeoutTests : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 1;
 
-        protected override bool CanStartServerAndClients() => false;
-
-        private bool m_UseSceneManagement;
-        public ConnectionApprovalTimeoutTests(bool useSceneManagement)
+        public enum ApprovalTimedOutTypes
         {
-            m_UseSceneManagement = useSceneManagement;
+            ClientDoesNotRequest,
+            ServerDoesNotRespond
+        }
+
+        private ApprovalTimedOutTypes m_ApprovalFailureType;
+
+        public ConnectionApprovalTimeoutTests(ApprovalTimedOutTypes approvalFailureType)
+        {
+            m_ApprovalFailureType = approvalFailureType;
         }
 
         // Must be >= 2 since this is an int value and the test waits for timeout - 1 to try to verify it doesn't
         // time out early
-        private const int k_TestTimeoutPeriod = 2;
+        private const int k_TestTimeoutPeriod = 1;
 
-        private void Start()
+        private Regex m_ExpectedLogMessage;
+        private LogType m_LogType;
+
+
+        protected override IEnumerator OnSetup()
         {
-            m_ServerNetworkManager.NetworkConfig.EnableSceneManagement = m_UseSceneManagement;
-            m_ClientNetworkManagers[0].NetworkConfig.EnableSceneManagement = m_UseSceneManagement;
-            if (!NetcodeIntegrationTestHelpers.Start(false, m_ServerNetworkManager, m_ClientNetworkManagers))
-            {
-                Debug.LogError("Failed to start instances");
-                Assert.Fail("Failed to start instances");
-            }
+            m_BypassConnectionTimeout = true;
+            return base.OnSetup();
         }
 
-        [UnityTest]
-        public IEnumerator WhenClientDoesntRequestApproval_ServerTimesOut()
+        protected override IEnumerator OnTearDown()
         {
-            Start();
-            var hook = new MessageCatcher<ConnectionRequestMessage>(m_ServerNetworkManager);
-            m_ServerNetworkManager.MessagingSystem.Hook(hook); ;
+            m_BypassConnectionTimeout = false;
+            return base.OnTearDown();
+        }
 
+        protected override void OnServerAndClientsCreated()
+        {
             m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
             m_ServerNetworkManager.LogLevel = LogLevel.Developer;
+            m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
             m_ClientNetworkManagers[0].LogLevel = LogLevel.Developer;
-
-            yield return new WaitForSeconds(m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout - 1);
-
-            Assert.AreEqual(0, m_ServerNetworkManager.ConnectedClients.Count);
-            Assert.AreEqual(1, m_ServerNetworkManager.PendingClients.Count);
-
-            var expectedLogMessage = new Regex($"Client {m_ServerNetworkManager.PendingClients.FirstOrDefault().Key} Handshake Timed Out");
-
-            NetcodeLogAssert.LogWasNotReceived(LogType.Log, expectedLogMessage);
-
-            yield return new WaitForSeconds(2);
-
-            NetcodeLogAssert.LogWasReceived(LogType.Log, expectedLogMessage);
-
-            Assert.AreEqual(0, m_ServerNetworkManager.ConnectedClients.Count);
-            Assert.AreEqual(0, m_ServerNetworkManager.PendingClients.Count);
+            base.OnServerAndClientsCreated();
         }
 
-        [UnityTest]
-        public IEnumerator WhenServerDoesntRespondWithApproval_ClientTimesOut()
+        protected override IEnumerator OnStartedServerAndClients()
         {
-            Start();
-
-            if (m_UseSceneManagement)
+            if (m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond)
             {
-                var sceneEventHook = new MessageCatcher<SceneEventMessage>(m_ClientNetworkManagers[0]);
-                m_ClientNetworkManagers[0].MessagingSystem.Hook(sceneEventHook);
+                // We catch (don't process) the incoming approval message to simulate the server not sending the approved message in time
+                m_ClientNetworkManagers[0].MessagingSystem.Hook(new MessageCatcher<ConnectionApprovedMessage>(m_ClientNetworkManagers[0]));
+                m_ExpectedLogMessage = new Regex("Timed out waiting for the server to approve the connection request.");
+                m_LogType = LogType.Log;
             }
             else
             {
-                var approvalHook = new MessageCatcher<ConnectionApprovedMessage>(m_ClientNetworkManagers[0]);
-                m_ClientNetworkManagers[0].MessagingSystem.Hook(approvalHook);
+                // 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.
+                m_ServerNetworkManager.MessagingSystem.Hook(new MessageCatcher<ConnectionRequestMessage>(m_ServerNetworkManager));
+
+                // For this test, we know the timed out client will be Client-1
+                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;
             }
+            yield return null;
+        }
 
-            m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
-            m_ServerNetworkManager.LogLevel = LogLevel.Developer;
-            m_ClientNetworkManagers[0].LogLevel = LogLevel.Developer;
+        [UnityTest]
+        public IEnumerator ValidateApprovalTimeout()
+        {
+            // Delay for half of the wait period
+            yield return new WaitForSeconds(k_TestTimeoutPeriod * 0.5f);
 
-            yield return new WaitForSeconds(m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout - 1);
+            // Verify we haven't received the time out message yet
+            NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage);
 
-            Assert.IsFalse(m_ClientNetworkManagers[0].IsConnectedClient);
-            Assert.IsTrue(m_ClientNetworkManagers[0].IsListening);
+            // 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);
 
-            var expectedLogMessage = new Regex("Server Handshake Timed Out");
-            NetcodeLogAssert.LogWasNotReceived(LogType.Log, expectedLogMessage);
+            // We should have the test relative log message by this time.
+            NetcodeLogAssert.LogWasReceived(m_LogType, m_ExpectedLogMessage);
 
-            yield return new WaitForSeconds(2);
-
-            NetcodeLogAssert.LogWasReceived(LogType.Log, expectedLogMessage);
-
-            Assert.IsFalse(m_ClientNetworkManagers[0].IsConnectedClient);
-            Assert.IsFalse(m_ClientNetworkManagers[0].IsListening);
+            // 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!");
+            Assert.AreEqual(0, m_ServerNetworkManager.PendingClients.Count, $"Expected no pending clients when there were {m_ServerNetworkManager.PendingClients.Count} pending clients!");
+            Assert.True(!m_ClientNetworkManagers[0].IsApproved, $"Expected the client to not have been approved, but it was!");
         }
     }
 }