fix: [Backport] Ensure NetworkConnectionManager correctly handles multiple disconnect messages being sent

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,7 +13,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
-* Changed minimum Unity version supported to 2022.3 LTS
+- The `NetworkManager` functions `GetTransportIdFromClientId` and `GetClientIdFromTransportId` will now return `ulong.MaxValue` when the clientId or transportId do not exist. (#3721)
+- Changed minimum Unity version supported to 2022.3 LTS
 
 ### Deprecated
 
@@ -23,6 +24,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Multiple disconnect events from the same transport will no longer disconnect the host. (#3721)
 
 ### Security
 

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

@@ -317,45 +317,45 @@ internal void RemovePendingClient(ulong clientId)
         private ulong m_NextClientId = 1;
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal ulong TransportIdToClientId(ulong transportId)
+        internal (ulong, bool) TransportIdToClientId(ulong transportId)
         {
             if (transportId == GetServerTransportId())
             {
-                return NetworkManager.ServerClientId;
+                return (NetworkManager.ServerClientId, true);
             }
 
             if (TransportIdToClientIdMap.TryGetValue(transportId, out var clientId))
             {
-                return clientId;
+                return (clientId, true);
             }
 
             if (NetworkLog.CurrentLogLevel == LogLevel.Developer)
             {
                 NetworkLog.LogWarning($"Trying to get the NGO client ID map for the transport ID ({transportId}) but did not find the map entry! Returning default transport ID value.");
             }
 
-            return default;
+            return (0, false);
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal ulong ClientIdToTransportId(ulong clientId)
+        internal (ulong, bool) ClientIdToTransportId(ulong clientId)
         {
             if (clientId == NetworkManager.ServerClientId)
             {
-                return GetServerTransportId();
+                return (GetServerTransportId(), true);
             }
 
             if (ClientIdToTransportIdMap.TryGetValue(clientId, out var transportClientId))
             {
-                return transportClientId;
+                return (transportClientId, true);
             }
 
             if (NetworkLog.CurrentLogLevel == LogLevel.Developer)
             {
                 NetworkLog.LogWarning($"Trying to get the transport client ID map for the NGO client ID ({clientId}) but did not find the map entry! Returning default transport ID value.");
             }
 
-            return default;
+            return (0, false);
         }
 
         /// <summary>
@@ -384,19 +384,24 @@ private ulong GetServerTransportId()
         /// Handles cleaning up the transport id/client id tables after receiving a disconnect event from transport.
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal ulong TransportIdCleanUp(ulong transportId)
+        internal (ulong, bool) TransportIdCleanUp(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 (!LocalClient.IsServer && !TransportIdToClientIdMap.ContainsKey(transportId))
             {
-                return NetworkManager.LocalClientId;
+                return (NetworkManager.LocalClientId, true);
+            }
+
+            var (clientId, isConnectedClient) = TransportIdToClientId(transportId);
+            if (!isConnectedClient)
+            {
+                return (default, false);
             }
 
-            var clientId = TransportIdToClientId(transportId);
             TransportIdToClientIdMap.Remove(transportId);
             ClientIdToTransportIdMap.Remove(clientId);
-            return clientId;
+            return (clientId, true);
         }
 
         internal void PollAndHandleNetworkEvents()
@@ -502,8 +507,11 @@ internal void DataEventHandler(ulong transportClientId, ref ArraySegment<byte> p
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
             s_HandleIncomingData.Begin();
 #endif
-            var clientId = TransportIdToClientId(transportClientId);
-            MessageManager.HandleIncomingData(clientId, payload, receiveTime);
+            var (clientId, isConnectedClient) = TransportIdToClientId(transportClientId);
+            if (isConnectedClient)
+            {
+                MessageManager.HandleIncomingData(clientId, payload, receiveTime);
+            }
 
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
             s_HandleIncomingData.End();
@@ -515,10 +523,15 @@ internal void DataEventHandler(ulong transportClientId, ref ArraySegment<byte> p
         /// </summary>
         internal void DisconnectEventHandler(ulong transportClientId)
         {
+            var (clientId, wasConnectedClient) = TransportIdCleanUp(transportClientId);
+            if (!wasConnectedClient)
+            {
+                return;
+            }
+
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
             s_TransportDisconnect.Begin();
 #endif
-            var clientId = TransportIdCleanUp(transportClientId);
             if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
             {
                 NetworkLog.LogInfo($"Disconnect Event From {clientId}");
@@ -1040,9 +1053,9 @@ internal void OnClientDisconnectFromServer(ulong clientId)
             }
 
             // If the client ID transport map exists
-            if (ClientIdToTransportIdMap.ContainsKey(clientId))
+            var (transportId, isConnected) = ClientIdToTransportId(clientId);
+            if (isConnected)
             {
-                var transportId = ClientIdToTransportId(clientId);
                 NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId);
 
                 InvokeOnClientDisconnectCallback(clientId);

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

@@ -1156,15 +1156,27 @@ private void HostServerInitialize()
         /// Get the TransportId from the associated ClientId.
         /// </summary>
         /// <param name="clientId">The ClientId to get the TransportId from</param>
-        /// <returns>The TransportId associated with the given ClientId</returns>
-        public ulong GetTransportIdFromClientId(ulong clientId) => ConnectionManager.ClientIdToTransportId(clientId);
+        /// <returns>
+        /// The TransportId associated with the given ClientId if the given clientId is valid; otherwise <see cref="ulong.MaxValue"/>
+        /// </returns>
+        public ulong GetTransportIdFromClientId(ulong clientId)
+        {
+            var (id, success) = ConnectionManager.ClientIdToTransportId(clientId);
+            return success ? id : ulong.MaxValue;
+        }
 
         /// <summary>
         /// Get the ClientId from the associated TransportId.
         /// </summary>
         /// <param name="transportId">The TransportId to get the ClientId from</param>
-        /// <returns>The ClientId from the associated TransportId</returns>
-        public ulong GetClientIdFromTransportId(ulong transportId) => ConnectionManager.TransportIdToClientId(transportId);
+        /// <returns>
+        /// The ClientId from the associated TransportId if the given transportId is valid; otherwise <see cref="ulong.MaxValue"/>
+        /// </returns>
+        public ulong GetClientIdFromTransportId(ulong transportId)
+        {
+            var (id, success) = ConnectionManager.TransportIdToClientId(transportId);
+            return success ? id : ulong.MaxValue;
+        }
 
         /// <summary>
         /// Disconnects the remote client.

com.unity.netcode.gameobjects/Runtime/Messaging/DefaultMessageSender.cs

@@ -19,8 +19,19 @@ public DefaultMessageSender(NetworkManager manager)
         public void Send(ulong clientId, NetworkDelivery delivery, FastBufferWriter batchData)
         {
             var sendBuffer = batchData.ToTempByteArray();
+            var (transportId, clientExists) = m_ConnectionManager.ClientIdToTransportId(clientId);
 
-            m_NetworkTransport.Send(m_ConnectionManager.ClientIdToTransportId(clientId), sendBuffer, delivery);
+            if (!clientExists)
+            {
+                if (m_ConnectionManager.NetworkManager.LogLevel <= LogLevel.Error)
+                {
+                    NetworkLog.LogWarning("Trying to send a message to a client who doesn't have a transport connection");
+                }
+
+                return;
+            }
+
+            m_NetworkTransport.Send(transportId, sendBuffer, delivery);
         }
     }
 }

com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs

@@ -868,7 +868,11 @@ private void SendBatchedMessages(SendTarget sendTarget, BatchedSendQueue queue)
             var mtu = 0;
             if (NetworkManager)
             {
-                var ngoClientId = NetworkManager.ConnectionManager.TransportIdToClientId(sendTarget.ClientId);
+                var (ngoClientId, isConnectedClient) = NetworkManager.ConnectionManager.TransportIdToClientId(sendTarget.ClientId);
+                if (!isConnectedClient)
+                {
+                    return;
+                }
                 mtu = NetworkManager.GetPeerMTU(ngoClientId);
             }
 
@@ -1278,7 +1282,7 @@ public override ulong GetCurrentRtt(ulong clientId)
 
             if (NetworkManager != null)
             {
-                var transportId = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
+                var (transportId, _) = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
 
                 var rtt = ExtractRtt(ParseClientId(transportId));
                 if (rtt > 0)
@@ -1329,9 +1333,9 @@ public NetworkEndpoint GetEndpoint(ulong clientId)
         {
             if (m_Driver.IsCreated && NetworkManager != null && NetworkManager.IsListening)
             {
-                var transportId = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
+                var (transportId, connectionExists) = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
                 var networkConnection = ParseClientId(transportId);
-                if (m_Driver.GetConnectionState(networkConnection) == NetworkConnection.State.Connected)
+                if (connectionExists && m_Driver.GetConnectionState(networkConnection) == NetworkConnection.State.Connected)
                 {
                     return m_Driver.RemoteEndPoint(networkConnection);
                 }
@@ -1460,10 +1464,17 @@ public override void Send(ulong clientId, ArraySegment<byte> payload, NetworkDel
                     // If the message is sent reliably, then we're over capacity and we can't
                     // provide any reliability guarantees anymore. Disconnect the client since at
                     // this point they're bound to become desynchronized.
+                    if (NetworkManager != null)
+                    {
+                        var (ngoClientId, isConnectedClient) = NetworkManager.ConnectionManager.TransportIdToClientId(clientId);
+                        if (isConnectedClient)
+                        {
+                            clientId = ngoClientId;
+                        }
 
-                    var ngoClientId = NetworkManager?.ConnectionManager.TransportIdToClientId(clientId) ?? clientId;
+                    }
                     Debug.LogError($"Couldn't add payload of size {payload.Count} to reliable send queue. " +
-                        $"Closing connection {ngoClientId} as reliability guarantees can't be maintained.");
+                        $"Closing connection {clientId} as reliability guarantees can't be maintained.");
 
                     if (clientId == m_ServerClientId)
                     {

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

@@ -287,6 +287,17 @@ public enum HostOrServer
         /// </summary>
         protected virtual bool m_EnableTimeTravel => false;
 
+        /// <summary>
+        /// When true, <see cref="CreateServerAndClients()"/> and <see cref="CreateNewClient"/> will use a <see cref="MockTransport"/>
+        /// as the <see cref="NetworkConfig.NetworkTransport"/> on the created server and/or clients.
+        /// When false, a <see cref="UnityTransport"/> is used.
+        /// </summary>
+        /// <remarks>
+        /// This defaults to, and is required to be true when <see cref="m_EnableTimeTravel"/> is true.
+        /// <see cref="m_EnableTimeTravel"/> will not work with the <see cref="UnityTransport"/> component.
+        /// </remarks>
+        protected virtual bool m_UseMockTransport => m_EnableTimeTravel;
+
         /// <summary>
         /// If this is false, SetUp will call OnInlineSetUp instead of OnSetUp.
         /// This is a performance advantage when not using the coroutine functionality, as a coroutine that
@@ -407,7 +418,7 @@ public IEnumerator SetUp()
         {
             VerboseDebug($"Entering {nameof(SetUp)}");
             NetcodeLogAssert = new NetcodeLogAssert();
-            if (m_EnableTimeTravel)
+            if (m_UseMockTransport)
             {
                 if (m_NetworkManagerInstatiationMode == NetworkManagerInstatiationMode.AllTests)
                 {
@@ -417,9 +428,14 @@ public IEnumerator SetUp()
                 {
                     MockTransport.Reset();
                 }
-                // Setup the frames per tick for time travel advance to next tick
+            }
+
+            // Setup the frames per tick for time travel advance to next tick
+            if (m_EnableTimeTravel)
+            {
                 ConfigureFramesPerTick();
             }
+
             if (m_SetupIsACoroutine)
             {
                 yield return OnSetup();
@@ -577,7 +593,7 @@ protected virtual bool ShouldWaitForNewClientToConnect(NetworkManager networkMan
         /// <returns>An IEnumerator for use with Unity's coroutine system.</returns>
         protected IEnumerator CreateAndStartNewClient()
         {
-            var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_EnableTimeTravel);
+            var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_UseMockTransport);
             networkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
 
             // Notification that the new client (NetworkManager) has been created
@@ -619,7 +635,7 @@ protected IEnumerator CreateAndStartNewClient()
         /// </summary>
         protected void CreateAndStartNewClientWithTimeTravel()
         {
-            var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_EnableTimeTravel);
+            var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_UseMockTransport);
             networkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
 
             // Notification that the new client (NetworkManager) has been created
@@ -732,7 +748,7 @@ protected void CreateServerAndClients(int numberOfClients)
             }
 
             // Create multiple NetworkManager instances
-            if (!NetcodeIntegrationTestHelpers.Create(numberOfClients, out NetworkManager server, out NetworkManager[] clients, m_TargetFrameRate, m_CreateServerFirst, m_EnableTimeTravel))
+            if (!NetcodeIntegrationTestHelpers.Create(numberOfClients, out NetworkManager server, out NetworkManager[] clients, m_TargetFrameRate, m_CreateServerFirst, m_UseMockTransport))
             {
                 Debug.LogError("Failed to create instances");
                 Assert.Fail("Failed to create instances");
@@ -1219,7 +1235,7 @@ public IEnumerator TearDown()
             VerboseDebug($"Exiting {nameof(TearDown)}");
             LogWaitForMessages();
             NetcodeLogAssert.Dispose();
-            if (m_EnableTimeTravel)
+            if (m_UseMockTransport)
             {
                 if (m_NetworkManagerInstatiationMode == NetworkManagerInstatiationMode.AllTests)
                 {

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

@@ -108,16 +108,24 @@ private void OnConnectionEvent(NetworkManager networkManager, ConnectionEventDat
         /// </summary>
         private bool TransportIdCleanedUp()
         {
-            if (m_ServerNetworkManager.ConnectionManager.TransportIdToClientId(m_TransportClientId) == m_ClientId)
+            var (clientId, isConnected) = m_ServerNetworkManager.ConnectionManager.TransportIdToClientId(m_TransportClientId);
+            if (isConnected)
             {
                 return false;
             }
 
-            if (m_ServerNetworkManager.ConnectionManager.ClientIdToTransportId(m_ClientId) == m_TransportClientId)
+            if (clientId == m_ClientId)
             {
                 return false;
             }
-            return true;
+
+            var (transportId, connectionExists) = m_ServerNetworkManager.ConnectionManager.ClientIdToTransportId(m_ClientId);
+            if (connectionExists)
+            {
+                return false;
+            }
+
+            return transportId != m_TransportClientId;
         }
 
         /// <summary>
@@ -144,7 +152,9 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client
 
             var serverSideClientPlayer = m_ServerNetworkManager.ConnectionManager.ConnectedClients[m_ClientId].PlayerObject;
 
-            m_TransportClientId = m_ServerNetworkManager.ConnectionManager.ClientIdToTransportId(m_ClientId);
+            bool connectionExists;
+            (m_TransportClientId, connectionExists) = m_ServerNetworkManager.ConnectionManager.ClientIdToTransportId(m_ClientId);
+            Assert.IsTrue(connectionExists);
 
             var clientManager = m_ClientNetworkManagers[0];