fix: Ensure transport connection events don't act if duplicated (#3863)

* fix: Ensure transport connection events don't act if duplicated

* Update CHANGELOG

* Fix the expected error message in tests

* reset MockTransport on shutdown

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -28,6 +28,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Duplicate transport connection events for the same connection will now do nothing. (#3863)
 - Fixed memory leak in `NetworkAnimator` on clients where `RpcTarget` groups were not being properly disposed due to incorrect type casting of `ProxyRpcTargetGroup` to `RpcTargetGroup`.
 - Fixed issue when using a client-server topology where a `NetworkList` with owner write permissions was resetting sent time and dirty flags after having been spawned on owning clients that were not the spawn authority. (#3850)
 - Fixed an integer overflow that occurred when configuring a large disconnect timeout with Unity Transport. (#3810)

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

@@ -494,10 +494,12 @@ internal void HandleNetworkEvent(NetworkEvent networkEvent, ulong transportClien
 
         internal ulong LocalClientTransportId => m_LocalClientTransportId;
 
+        private bool m_IsTransportConnected = false;
+
         /// <summary>
         /// Handles a <see cref="NetworkEvent.Connect"/> event.
         /// </summary>
-        internal void ConnectEventHandler(ulong transportClientId)
+        internal void ConnectEventHandler(ulong transportId)
         {
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
             s_TransportConnect.Begin();
@@ -507,20 +509,45 @@ internal void ConnectEventHandler(ulong transportClientId)
             // - When client receives one, it *must be* the server
             // Client's can't connect to or talk to other clients.
             // Server is a sentinel so only one exists, if we are server, we can't be connecting to it.
-            var clientId = transportClientId;
+
+            ulong clientId;
+
+            // If we're the server, then this connect event is firing for a new incoming client connection
             if (LocalClient.IsServer)
             {
+                var (_, alreadyConnected) = TransportIdToClientId(transportId);
+                if (alreadyConnected)
+                {
+                    if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
+                    {
+                        NetworkLog.LogError($"[TransportApproval][Server] TransportId {transportId} is already connected to this server!");
+                    }
+                    return;
+                }
+
                 clientId = m_NextClientId++;
             }
+            // Otherwise this connect event is an approved connection from the server
             else
             {
+                if (m_IsTransportConnected)
+                {
+                    if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
+                    {
+                        NetworkLog.LogError("[TransportApproval][Client] Client received a transport connection event after already connecting!");
+                    }
+                    return;
+                }
+
+                m_IsTransportConnected = true;
+
                 // Cache the local client's transport id.
-                m_LocalClientTransportId = transportClientId;
+                m_LocalClientTransportId = transportId;
                 clientId = NetworkManager.ServerClientId;
             }
 
-            ClientIdToTransportIdMap[clientId] = transportClientId;
-            TransportIdToClientIdMap[transportClientId] = clientId;
+            ClientIdToTransportIdMap[clientId] = transportId;
+            TransportIdToClientIdMap[transportId] = clientId;
             MessageManager.ClientConnected(clientId);
 
             if (LocalClient.IsServer)
@@ -1540,6 +1567,7 @@ internal void Initialize(NetworkManager networkManager)
             ConnectedClientIds.Clear();
             ClientIdToTransportIdMap.Clear();
             TransportIdToClientIdMap.Clear();
+            m_IsTransportConnected = false;
             ClientsToApprove.Clear();
             NetworkObject.OrphanChildren.Clear();
             m_DisconnectReason = string.Empty;

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

@@ -104,7 +104,7 @@ public bool OnVerifyCanReceive(ulong senderId, Type messageType, FastBufferReade
                     if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                     {
                         var transportErrorMsg = GetTransportErrorMessage(messageContent, m_NetworkManager);
-                        NetworkLog.LogError($"A {nameof(ConnectionApprovedMessage)} was received from the server when the connection has already been established. {transportErrorMsg}");
+                        NetworkLog.LogError($"A {nameof(ConnectionApprovedMessage)} was received from the server when a connection has already been established. {transportErrorMsg}");
                     }
 
                     return false;
@@ -118,11 +118,11 @@ private static string GetTransportErrorMessage(FastBufferReader messageContent,
         {
             if (networkManager.NetworkConfig.NetworkTransport is not UnityTransport)
             {
-                return $"NetworkTransport: {networkManager.NetworkConfig.NetworkTransport.GetType()}. Please report this to the maintainer of transport layer.";
+                return $"NetworkTransport: {networkManager.NetworkConfig.NetworkTransport.GetType()}. Please report this to the maintainer of the transport layer.";
             }
 
             var transportVersion = GetTransportVersion(networkManager);
-            return $"{transportVersion}. This should not happen. Please report this to the Netcode for GameObjects team at https://github.com/Unity-Technologies/com.unity.netcode.gameobjects/issues and include the following data: Message Size: {messageContent.Length}. Message Content: {NetworkMessageManager.ByteArrayToString(messageContent.ToArray(), 0, messageContent.Length)}";
+            return $"{transportVersion}. This should not happen. Further information: Message Size: {messageContent.Length}. Message Content: {NetworkMessageManager.ByteArrayToString(messageContent.ToArray(), 0, messageContent.Length)}";
         }
 
         private static string GetTransportVersion(NetworkManager networkManager)

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

@@ -96,7 +96,7 @@ public IEnumerator WhenSendingConnectionApprovedToAlreadyConnectedClient_Connect
 
             m_ClientNetworkManagers[0].ConnectionManager.MessageManager.Hook(new Hooks<ConnectionApprovedMessage>());
 
-            LogAssert.Expect(LogType.Error, new Regex($"A {nameof(ConnectionApprovedMessage)} was received from the server when the connection has already been established\\. NetworkTransport: Unity.Netcode.Transports.UTP.UnityTransport UnityTransportProtocol: UnityTransport. This should not happen\\."));
+            LogAssert.Expect(LogType.Error, new Regex($"A {nameof(ConnectionApprovedMessage)} was received from the server when a connection has already been established\\. NetworkTransport: Unity.Netcode.Transports.UTP.UnityTransport UnityTransportProtocol: UnityTransport. This should not happen\\."));
 
             yield return WaitForMessageReceived<UnnamedMessage>(m_ClientNetworkManagers.ToList());
         }

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

@@ -19,7 +19,7 @@ private struct MessageData
         public override ulong ServerClientId { get; } = 0;
 
         public static ulong HighTransportId = 0;
-        public ulong TransportId = 0;
+        public ulong TransportId = ulong.MaxValue;
         public float SimulatedLatencySeconds;
         public float PacketDropRate;
         public float LatencyJitter;
@@ -75,15 +75,23 @@ public override NetworkEvent PollEvent(out ulong clientId, out ArraySegment<byte
 
         public override bool StartClient()
         {
-            TransportId = ++HighTransportId;
-            s_MessageQueue[TransportId] = new Queue<MessageData>();
+            // TransportId will be the max if it hasn't already been set.
+            if (TransportId == ulong.MaxValue)
+            {
+                TransportId = ++HighTransportId;
+                s_MessageQueue[TransportId] = new Queue<MessageData>();
+            }
             s_MessageQueue[ServerClientId].Enqueue(new MessageData { Event = NetworkEvent.Connect, FromClientId = TransportId, Payload = new ArraySegment<byte>() });
             return true;
         }
 
         public override bool StartServer()
         {
-            s_MessageQueue[ServerClientId] = new Queue<MessageData>();
+            if (TransportId == ulong.MaxValue)
+            {
+                TransportId = ServerClientId;
+                s_MessageQueue[ServerClientId] = new Queue<MessageData>();
+            }
             return true;
         }
 
@@ -104,6 +112,7 @@ public override ulong GetCurrentRtt(ulong clientId)
 
         public override void Shutdown()
         {
+            TransportId = ulong.MaxValue;
         }
 
         protected override NetworkTopologyTypes OnCurrentTopology()

com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs

@@ -40,8 +40,27 @@ public IEnumerator MultipleDisconnectEventsNoop()
             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}");
+        }
+
+        [UnityTest]
+        public IEnumerator MultipleConnectMessagesNoop()
+        {
+            var clientToConnect = GetNonAuthorityNetworkManager(0);
+            var clientTransport = clientToConnect.NetworkConfig.NetworkTransport;
+
+            var otherClient = GetNonAuthorityNetworkManager(1);
 
+            var currentConnectedClients = m_UseHost ? NumberOfClients + 1 : NumberOfClients;
 
+            clientTransport.StartClient();
+            clientTransport.StartClient();
+
+            // Start a new client to ensure everything is still working
+            yield return CreateAndStartNewClient();
+
+            var newExpectedClients = currentConnectedClients + 1;
+            yield return WaitForConditionOrTimeOut(() => otherClient.ConnectedClientsIds.Count == newExpectedClients);
+            AssertOnTimeout($"Incorrect number of connected clients. Expected: {newExpectedClients}, have: {otherClient.ConnectedClientsIds.Count}");
         }
     }
 }