diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 38656d18cd..aedadc9759 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/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) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 549cfe2059..058fb6a38b 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/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; + /// /// Handles a event. /// - 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; diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkManagerHooks.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkManagerHooks.cs index 05f0d2a2f0..cd91b177ab 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkManagerHooks.cs +++ b/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) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/InvalidConnectionEventsTest.cs b/com.unity.netcode.gameobjects/Tests/Runtime/InvalidConnectionEventsTest.cs index eafe1b5cd1..7cabfbff36 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/InvalidConnectionEventsTest.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/InvalidConnectionEventsTest.cs @@ -96,7 +96,7 @@ public IEnumerator WhenSendingConnectionApprovedToAlreadyConnectedClient_Connect m_ClientNetworkManagers[0].ConnectionManager.MessageManager.Hook(new Hooks()); - 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(m_ClientNetworkManagers.ToList()); } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/MockTransport.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/MockTransport.cs index 587f2990a3..ea97cf1233 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/MockTransport.cs +++ b/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(); + // TransportId will be the max if it hasn't already been set. + if (TransportId == ulong.MaxValue) + { + TransportId = ++HighTransportId; + s_MessageQueue[TransportId] = new Queue(); + } s_MessageQueue[ServerClientId].Enqueue(new MessageData { Event = NetworkEvent.Connect, FromClientId = TransportId, Payload = new ArraySegment() }); return true; } public override bool StartServer() { - s_MessageQueue[ServerClientId] = new Queue(); + if (TransportId == ulong.MaxValue) + { + TransportId = ServerClientId; + s_MessageQueue[ServerClientId] = new Queue(); + } return true; } @@ -104,6 +112,7 @@ public override ulong GetCurrentRtt(ulong clientId) public override void Shutdown() { + TransportId = ulong.MaxValue; } protected override NetworkTopologyTypes OnCurrentTopology() diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs index c5a7b6c039..5eb3b542e1 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs +++ b/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}"); } } }