fix: Ensure NetworkConnectionManager correctly handles multiple disconnect messages being sent (#3707)

* fix: Ensure NetworkConnectionManager correctly handles multiple disconnect messages being sent

* small fixes

* update CHANGELOG

* Remove debug logs and fix xml doc

* Update CHANGELOG with info for #3647

* Undo update to CHANGELOG

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,6 +13,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
+- The `NetworkManager` functions `GetTransportIdFromClientId` and `GetClientIdFromTransportId` will now return `ulong.MaxValue` when the clientId or transportId do not exist. (#3707)
+- Improved performance of the NetworkVariable. (#3683)
+- Improved performance around the NetworkBehaviour component. (#3687)
 
 ### Deprecated
 
@@ -22,6 +25,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Multiple disconnect events from the same transport will no longer disconnect the host. (#3707)
+- Distributed authority clients no longer send themselves in the `ClientIds` list when sending a `ChangeOwnershipMessage`. (#3687)
+- Made a variety of small performance improvements. (#3683)
 
 ### Security
 

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

@@ -1441,18 +1441,13 @@ private void HostServerInitialize()
                     }
                 }
 
-                response.Approved = true;
-                ConnectionManager.HandleConnectionApproval(ServerClientId, response);
+                ConnectionManager.HandleConnectionApproval(ServerClientId, response.CreatePlayerObject, response.PlayerPrefabHash, response.Position, response.Rotation);
             }
             else
             {
-                var response = new ConnectionApprovalResponse
-                {
-                    Approved = true,
-                    // Distributed authority always returns true since the client side handles spawning (whether automatically or manually)
-                    CreatePlayerObject = DistributedAuthorityMode || NetworkConfig.PlayerPrefab != null,
-                };
-                ConnectionManager.HandleConnectionApproval(ServerClientId, response);
+                // Distributed authority always tries to create the player object since the client side handles spawning (whether automatically or manually)
+                var createPlayerObject = DistributedAuthorityMode || NetworkConfig.PlayerPrefab != null;
+                ConnectionManager.HandleConnectionApproval(ServerClientId, createPlayerObject);
             }
 
             SpawnManager.ServerSpawnSceneObjectsOnStartSweep();
@@ -1473,15 +1468,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/Core/NetworkManager.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/Messaging/DefaultMessageSender.cs

@@ -210,12 +210,16 @@ public void Handle(ref NetworkContext context)
             }
             else
             {
-                var response = new NetworkManager.ConnectionApprovalResponse
+                var createPlayerObject = networkManager.NetworkConfig.PlayerPrefab != null;
+
+                // DAHost only:
+                // Never create the player object on the server if AutoSpawnPlayerPrefabClientSide is set.
+                if (networkManager.DistributedAuthorityMode && networkManager.AutoSpawnPlayerPrefabClientSide)
                 {
-                    Approved = true,
-                    CreatePlayerObject = networkManager.DistributedAuthorityMode && networkManager.AutoSpawnPlayerPrefabClientSide ? false : networkManager.NetworkConfig.PlayerPrefab != null
-                };
-                networkManager.ConnectionManager.HandleConnectionApproval(senderId, response);
+                    createPlayerObject = false;
+                }
+
+                networkManager.ConnectionManager.HandleConnectionApproval(senderId, createPlayerObject);
             }
         }
     }

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

@@ -909,7 +909,11 @@ private void SendBatchedMessages(SendTarget sendTarget, BatchedSendQueue queue)
             var mtu = 0;
             if (m_NetworkManager)
             {
-                var ngoClientId = m_NetworkManager.ConnectionManager.TransportIdToClientId(sendTarget.ClientId);
+                var (ngoClientId, isConnectedClient) = m_NetworkManager.ConnectionManager.TransportIdToClientId(sendTarget.ClientId);
+                if (!isConnectedClient)
+                {
+                    return;
+                }
                 mtu = m_NetworkManager.GetPeerMTU(ngoClientId);
             }
 
@@ -1321,7 +1325,7 @@ public override ulong GetCurrentRtt(ulong clientId)
 
             if (m_NetworkManager != null)
             {
-                var transportId = m_NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
+                var (transportId, _) = m_NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
 
                 var rtt = ExtractRtt(ParseClientId(transportId));
                 if (rtt > 0)
@@ -1347,13 +1351,14 @@ public NetworkEndpoint GetEndpoint(ulong clientId)
         {
             if (m_Driver.IsCreated && m_NetworkManager != null && m_NetworkManager.IsListening)
             {
-                var transportId = m_NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
+                var (transportId, connectionExists) = m_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.GetRemoteEndpoint(networkConnection);
                 }
             }
+
             return new NetworkEndpoint();
         }
 
@@ -1447,10 +1452,18 @@ 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 (m_NetworkManager != null)
+                    {
+                        var (ngoClientId, isConnectedClient) = m_NetworkManager.ConnectionManager.TransportIdToClientId(clientId);
+                        if (isConnectedClient)
+                        {
+                            clientId = ngoClientId;
+                        }
+
+                    }
 
-                    var ngoClientId = m_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/Runtime/Transports/UTP/UnityTransport.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>
@@ -145,7 +153,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);
 
             if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient)
             {

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

@@ -484,6 +484,17 @@ protected void SetDistributedAuthorityProperties(NetworkManager networkManager)
         /// </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
@@ -638,7 +649,7 @@ public IEnumerator SetUp()
             VerboseDebugLog.Clear();
             VerboseDebug($"Entering {nameof(SetUp)}");
             NetcodeLogAssert = new NetcodeLogAssert();
-            if (m_EnableTimeTravel)
+            if (m_UseMockTransport)
             {
                 if (m_NetworkManagerInstatiationMode == NetworkManagerInstatiationMode.AllTests)
                 {
@@ -648,8 +659,11 @@ 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();
             }
 
@@ -781,7 +795,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, m_UseCmbService))
+            if (!NetcodeIntegrationTestHelpers.Create(numberOfClients, out NetworkManager server, out NetworkManager[] clients, m_TargetFrameRate, m_CreateServerFirst, m_UseMockTransport, m_UseCmbService))
             {
                 Debug.LogError("Failed to create instances");
                 Assert.Fail("Failed to create instances");
@@ -872,7 +886,7 @@ protected virtual bool ShouldWaitForNewClientToConnect(NetworkManager networkMan
         /// <returns>The newly created <see cref="NetworkManager"/>.</returns>
         protected NetworkManager CreateNewClient()
         {
-            var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_EnableTimeTravel, m_UseCmbService);
+            var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length, m_UseMockTransport, m_UseCmbService);
             networkManager.NetworkConfig.PlayerPrefab = m_PlayerPrefab;
             SetDistributedAuthorityProperties(networkManager);
 
@@ -981,7 +995,7 @@ private bool AllPlayerObjectClonesSpawned(NetworkManager joinedClient)
         /// </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;
             SetDistributedAuthorityProperties(networkManager);
 

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

@@ -327,7 +327,18 @@ public static bool Create(int clientCount, out NetworkManager server, out Networ
             return true;
         }
 
-        internal static NetworkManager CreateNewClient(int identifier, bool mockTransport = false, bool useCmbService = false)
+        /// <summary>
+        /// Creates a new <see cref="NetworkManager"/> and configures it for use in a multi instance setting.
+        /// </summary>
+        /// <param name="identifier">The ClientId representation that is used in the name of the NetworkManager</param>
+        /// <param name="mockTransport">
+        /// When true, the client is created with a <see cref="MockTransport"/>; otherwise a <see cref="UnityTransport"/> is added
+        /// </param>
+        /// <param name="useCmbService">
+        /// Whether to configure the client to run against a hosted build of the CMB Service. Only applies if mockTransport is set to false.
+        /// </param>
+        /// <returns>The newly created <see cref="NetworkManager"/> component.</returns>
+        public static NetworkManager CreateNewClient(int identifier, bool mockTransport = false, bool useCmbService = false)
         {
             // Create gameObject
             var go = new GameObject("NetworkManager - Client - " + identifier);
@@ -351,7 +362,7 @@ internal static NetworkManager CreateNewClient(int identifier, bool mockTranspor
         /// <param name="clients">Output array containing the created NetworkManager instances</param>
         /// <param name="useMockTransport">When true, uses mock transport for testing, otherwise uses real transport. Default value is false</param>
         /// <param name="useCmbService">If true, each client will be created with transport configured to connect to a locally hosted da service</param>
-        /// <returns> Returns <see cref="true"/> if the clients were successfully created and configured, otherwise <see cref="false"/>.</returns>
+        /// <returns> Returns true if the clients were successfully created and configured, otherwise false.</returns>
         public static bool CreateNewClients(int clientCount, out NetworkManager[] clients, bool useMockTransport = false, bool useCmbService = false)
         {
             clients = new NetworkManager[clientCount];

com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/NetcodeIntegrationTestHelpers.cs

@@ -0,0 +1,47 @@
+using System.Collections;
+using NUnit.Framework;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    [TestFixture(HostOrServer.Server)]
+    [TestFixture(HostOrServer.Host)]
+    internal class TransportTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 2;
+
+        protected override bool m_UseMockTransport => true;
+
+        public TransportTests(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        [UnityTest]
+        public IEnumerator MultipleDisconnectEventsNoop()
+        {
+            var clientToDisconnect = GetNonAuthorityNetworkManager(0);
+            var clientTransport = clientToDisconnect.NetworkConfig.NetworkTransport;
+
+            var otherClient = GetNonAuthorityNetworkManager(1);
+
+            // Send multiple disconnect events
+            clientTransport.DisconnectLocalClient();
+            clientTransport.DisconnectLocalClient();
+
+            // completely stop and clean up the client
+            yield return StopOneClient(clientToDisconnect);
+
+            var expectedConnectedClients = m_UseHost ? NumberOfClients : NumberOfClients - 1;
+            yield return WaitForConditionOrTimeOut(() => otherClient.ConnectedClientsIds.Count == expectedConnectedClients);
+            AssertOnTimeout($"Incorrect number of connected clients. Expected: {expectedConnectedClients}, have: {otherClient.ConnectedClientsIds.Count}");
+
+            // Start a new client to ensure everything is still working
+            yield return CreateAndStartNewClient();
+
+            var newExpectedClients = m_UseHost ? NumberOfClients + 1 : NumberOfClients;
+            yield return WaitForConditionOrTimeOut(() => otherClient.ConnectedClientsIds.Count == newExpectedClients);
+            AssertOnTimeout($"Incorrect number of connected clients. Expected: {newExpectedClients}, have: {otherClient.ConnectedClientsIds.Count}");
+
+
+        }
+    }
+}