fix: key not found exception when client never connects (#1821)

* fix

This fixes the issue where if a client fails to connect after a specific period of time it will throw an exception because it never populated the m_TransportIdToClientIdMap.

* update

this is a better way of handling this

* test

The test for this fix where when a client cannot reach the server (i.e. times out) it would throw an exception complaining about a key not being found.  This test validates the fix for both UNet and UTP.

* Update CHANGELOG.md

* update

changing the default return to be zero when a client fails to connect.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -19,13 +19,14 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Removed `com.unity.modules.animation`, `com.unity.modules.physics` and `com.unity.modules.physics2d` dependencies from the package (#1812)
 
 ### Fixed
+- Fixed client throws a key not found exception when it times out using UNet or UTP. (#1821)
+- Fixed network variable updates are no longer limited to 32,768 bytes when NetworkConfig.EnsureNetworkVariableLengthSafety is enabled. The limits are now determined by what the transport can send in a message. (#1811)
 - Fixed in-scene NetworkObjects get destroyed if a client fails to connect and shuts down the NetworkManager. (#1809)
 - Fixed user never being notified in the editor that a NetworkBehaviour requires a NetworkObject to function properly. (#1808)
 - Fixed PlayerObjects and dynamically spawned NetworkObjects not being added to the NetworkClient's OwnedObjects (#1801)
 - Fixed issue where NetworkManager would continue starting even if the NetworkTransport selected failed. (#1780)
 - Fixed issue when spawning new player if an already existing player exists it does not remove IsPlayer from the previous player (#1779)
 - Fixed lack of notification that NetworkManager and NetworkObject cannot be added to the same GameObject with in-editor notifications (#1777)
-- Network variable updates are no longer limited to 32,768 bytes when NetworkConfig.EnsureNetworkVariableLengthSafety is enabled. The limits are now determined by what the transport can send in a message. (#1811)
 
 ## [1.0.0-pre.6] - 2022-03-02
 

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

@@ -12,6 +12,7 @@
 #endif
 using Unity.Profiling;
 using UnityEngine.SceneManagement;
+using System.Runtime.CompilerServices;
 using Debug = UnityEngine.Debug;
 
 namespace Unity.Netcode
@@ -1436,18 +1437,15 @@ private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, A
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
                     s_TransportDisconnect.Begin();
 #endif
-                    clientId = TransportIdToClientId(clientId);
-
-                    OnClientDisconnectCallback?.Invoke(clientId);
-
-                    m_TransportIdToClientIdMap.Remove(transportId);
-                    m_ClientIdToTransportIdMap.Remove(clientId);
+                    clientId = TransportIdCleanUp(clientId, transportId);
 
                     if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
                     {
                         NetworkLog.LogInfo($"Disconnect Event From {clientId}");
                     }
 
+                    OnClientDisconnectCallback?.Invoke(clientId);
+
                     if (IsServer)
                     {
                         OnClientDisconnectFromServer(clientId);
@@ -1463,6 +1461,31 @@ private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, A
             }
         }
 
+        /// <summary>
+        /// Handles cleaning up the transport id/client id tables after
+        /// receiving a disconnect event from transport
+        /// </summary>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private ulong TransportIdCleanUp(ulong clientId, ulong transportId)
+        {
+            // This check is for clients that attempted to connect but failed.
+            // When this happens, the client will not have an entry within the
+            // m_TransportIdToClientIdMap or m_ClientIdToTransportIdMap lookup
+            // tables so we exit early and just return 0 to be used for the
+            // disconnect event.
+            if (!IsServer && !m_TransportIdToClientIdMap.ContainsKey(clientId))
+            {
+                return 0;
+            }
+
+            clientId = TransportIdToClientId(clientId);
+
+            m_TransportIdToClientIdMap.Remove(transportId);
+            m_ClientIdToTransportIdMap.Remove(clientId);
+
+            return clientId;
+        }
+
         internal unsafe int SendMessage<TMessageType, TClientIdListType>(ref TMessageType message, NetworkDelivery delivery, in TClientIdListType clientIds)
             where TMessageType : INetworkMessage
             where TClientIdListType : IReadOnlyList<ulong>

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

@@ -0,0 +1,76 @@
+using System.Collections;
+using NUnit.Framework;
+using UnityEngine;
+using UnityEngine.TestTools;
+using Unity.Netcode.TestHelpers.Runtime;
+#if UNITY_UNET_PRESENT
+using Unity.Netcode.Transports.UNET;
+#endif
+
+namespace Unity.Netcode.RuntimeTests
+{
+    public class ClientOnlyConnectionTests
+    {
+        private NetworkManager m_ClientNetworkManager;
+        private GameObject m_NetworkManagerGameObject;
+        private WaitForSeconds m_DefaultWaitForTick = new WaitForSeconds(1.0f / 30);
+        private bool m_WasDisconnected;
+        private TimeoutHelper m_TimeoutHelper;
+
+        [SetUp]
+        public void Setup()
+        {
+            m_WasDisconnected = false;
+            m_NetworkManagerGameObject = new GameObject();
+            m_ClientNetworkManager = m_NetworkManagerGameObject.AddComponent<NetworkManager>();
+            m_ClientNetworkManager.NetworkConfig = new NetworkConfig();
+#if UNITY_UNET_PRESENT
+            m_TimeoutHelper = new TimeoutHelper(30);
+            m_ClientNetworkManager.NetworkConfig.NetworkTransport = m_NetworkManagerGameObject.AddComponent<UNetTransport>();
+#else
+            // Default is 1000ms per connection attempt and 60 connection attempts (60s)
+            // Currently there is no easy way to set these values other than in-editor
+            m_TimeoutHelper = new TimeoutHelper(70);
+            m_ClientNetworkManager.NetworkConfig.NetworkTransport = m_NetworkManagerGameObject.AddComponent<UnityTransport>();
+#endif
+        }
+
+        [UnityTest]
+        public IEnumerator ClientFailsToConnect()
+        {
+            // Wait for the disconnected event
+            m_ClientNetworkManager.OnClientDisconnectCallback += M_ClientNetworkManager_OnClientDisconnectCallback;
+
+            // Only start the client (so it will timeout)
+            m_ClientNetworkManager.StartClient();
+
+#if !UNITY_UNET_PRESENT
+            // Unity Transport throws an error when it times out
+            LogAssert.Expect(LogType.Error, "Failed to connect to server.");
+#endif
+            yield return NetcodeIntegrationTest.WaitForConditionOrTimeOut(() => m_WasDisconnected, m_TimeoutHelper);
+            Assert.False(m_TimeoutHelper.TimedOut, "Timed out waiting for client to timeout waiting to connect!");
+
+            // Shutdown the client
+            m_ClientNetworkManager.Shutdown();
+
+            // Wait for a tick
+            yield return m_DefaultWaitForTick;
+        }
+
+        private void M_ClientNetworkManager_OnClientDisconnectCallback(ulong obj)
+        {
+            m_WasDisconnected = true;
+        }
+
+        [TearDown]
+        public void TearDown()
+        {
+            if (m_NetworkManagerGameObject != null)
+            {
+                Object.DestroyImmediate(m_NetworkManagerGameObject);
+            }
+        }
+    }
+}
+