fix: NetworkManager ApprovalTimeout should not depend upon client synchronization (Unity-Technologies#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.

Author: NoelStephensUnity

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 @@ private void BuildDestinationToTransitionInfoTable()
         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 @@ public IReadOnlyList<ulong> ConnectedClientsIds
         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>
@@ -882,6 +894,8 @@ private void Initialize(bool server)
                 return;
             }
 
+            IsApproved = false;
+
             ComponentFactory.SetDefaults();
 
             if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
@@ -1025,7 +1039,6 @@ public bool StartServer()
             }
 
             Initialize(true);
-
             IsServer = true;
             IsClient = false;
             IsListening = true;
@@ -1038,6 +1051,7 @@ public bool StartServer()
                     SpawnManager.ServerSpawnSceneObjectsOnStartSweep();
 
                     OnServerStarted?.Invoke();
+                    IsApproved = true;
                     return true;
                 }
                 else
@@ -1159,6 +1173,7 @@ public bool StartHost()
                 }
 
                 response.Approved = true;
+                IsApproved = true;
                 HandleConnectionApproval(ServerClientId, response);
             }
             else
@@ -1420,6 +1435,7 @@ internal void ShutdownInternal()
             }
 
             IsConnectedClient = false;
+            IsApproved = false;
 
             // We need to clean up NetworkObjects before we reset the IsServer
             // and IsClient properties. This provides consistency of these two
@@ -1656,59 +1672,71 @@ private void SendConnectionRequest()
 
         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);
 
-                if (!IsListening)
-                {
-                    yield break;
+                    // 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 (PendingClients.ContainsKey(clientId) && !ConnectedClients.ContainsKey(clientId))
+                else
                 {
-                    // 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 (timedOut)
+                    {
+                        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 (!IsListening)
+                if (IsServer)
                 {
-                    yield break;
+                    DisconnectClient(clientId);
                 }
-
-                if (!IsConnectedClient)
+                else
                 {
-                    // Timeout
-                    if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
-                    {
-                        NetworkLog.LogInfo("Server Handshake Timed Out");
-                    }
                     Shutdown(true);
                 }
             }

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

@@ -79,6 +79,7 @@ public void Handle(ref NetworkContext context)
             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 @@ public enum HostOrServer
 
         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 @@ protected IEnumerator StartServerAndClients()
                 // 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();
+                    ClientNetworkManagerPostStartInit();
 
-                // Notification that at this time the server and client(s) are instantiated,
-                // started, and connected on both sides.
-                yield return OnServerAndClientsConnected();
+                    // 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)}");
+                    VerboseDebug($"Exiting {nameof(StartServerAndClients)}");
+                }
             }
         }