diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index d6270dadca..2dd4f9b434 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/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 diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index cc8796925e..5febc5e65f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -317,16 +317,16 @@ 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) @@ -334,20 +334,20 @@ internal ulong TransportIdToClientId(ulong transportId) 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) @@ -355,7 +355,7 @@ internal ulong ClientIdToTransportId(ulong clientId) 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); } /// @@ -384,19 +384,24 @@ private ulong GetServerTransportId() /// Handles cleaning up the transport id/client id tables after receiving a disconnect event from transport. /// [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 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 p /// 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); diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index eec6bc9b9e..242b019ec2 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -1156,15 +1156,27 @@ private void HostServerInitialize() /// Get the TransportId from the associated ClientId. /// /// The ClientId to get the TransportId from - /// The TransportId associated with the given ClientId - public ulong GetTransportIdFromClientId(ulong clientId) => ConnectionManager.ClientIdToTransportId(clientId); + /// + /// The TransportId associated with the given ClientId if the given clientId is valid; otherwise + /// + public ulong GetTransportIdFromClientId(ulong clientId) + { + var (id, success) = ConnectionManager.ClientIdToTransportId(clientId); + return success ? id : ulong.MaxValue; + } /// /// Get the ClientId from the associated TransportId. /// /// The TransportId to get the ClientId from - /// The ClientId from the associated TransportId - public ulong GetClientIdFromTransportId(ulong transportId) => ConnectionManager.TransportIdToClientId(transportId); + /// + /// The ClientId from the associated TransportId if the given transportId is valid; otherwise + /// + public ulong GetClientIdFromTransportId(ulong transportId) + { + var (id, success) = ConnectionManager.TransportIdToClientId(transportId); + return success ? id : ulong.MaxValue; + } /// /// Disconnects the remote client. diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/DefaultMessageSender.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/DefaultMessageSender.cs index 021ad0c604..873762fee8 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/DefaultMessageSender.cs +++ b/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); } } } diff --git a/com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs b/com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs index 39db9ae989..fddb41addd 100644 --- a/com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs +++ b/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) @@ -1290,7 +1294,6 @@ public override ulong GetCurrentRtt(ulong clientId) return (ulong)ExtractRtt(ParseClientId(clientId)); } -#if UTP_TRANSPORT_2_0_ABOVE /// /// Provides the for the NGO client identifier specified. /// @@ -1305,40 +1308,19 @@ 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) { +#if UTP_TRANSPORT_2_0_ABOVE return m_Driver.GetRemoteEndpoint(networkConnection); - } - } - return new NetworkEndpoint(); - } #else - /// - /// Provides the for the NGO client identifier specified. - /// - /// - /// - This is only really useful for direct connections. - /// - Relay connections and clients connected using a distributed authority network topology will not provide the client's actual endpoint information. - /// - For LAN topologies this should work as long as it is a direct connection and not a relay connection. - /// - /// NGO client identifier to get endpoint information about. - /// - public NetworkEndpoint GetEndpoint(ulong clientId) - { - if (m_Driver.IsCreated && NetworkManager != null && NetworkManager.IsListening) - { - var transportId = NetworkManager.ConnectionManager.ClientIdToTransportId(clientId); - var networkConnection = ParseClientId(transportId); - if (m_Driver.GetConnectionState(networkConnection) == NetworkConnection.State.Connected) - { return m_Driver.RemoteEndPoint(networkConnection); +#endif } } return new NetworkEndpoint(); } -#endif /// @@ -1460,10 +1442,17 @@ public override void Send(ulong clientId, ArraySegment 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) { diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs index 49e7142df7..1e6820c46b 100644 --- a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs @@ -287,6 +287,17 @@ public enum HostOrServer /// protected virtual bool m_EnableTimeTravel => false; + /// + /// When true, and will use a + /// as the on the created server and/or clients. + /// When false, a is used. + /// + /// + /// This defaults to, and is required to be true when is true. + /// will not work with the component. + /// + protected virtual bool m_UseMockTransport => m_EnableTimeTravel; + /// /// 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 /// An IEnumerator for use with Unity's coroutine system. 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() /// 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) { diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs index 1cc9bc40c9..d3faa724fc 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs @@ -108,16 +108,24 @@ private void OnConnectionEvent(NetworkManager networkManager, ConnectionEventDat /// 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; } /// @@ -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]; diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs new file mode 100644 index 0000000000..bc7a5a2044 --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs @@ -0,0 +1,45 @@ +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 = m_ClientNetworkManagers[0]; + var clientTransport = clientToDisconnect.NetworkConfig.NetworkTransport; + + var otherClient = m_ClientNetworkManagers[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}"); + } + } +} diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs.meta new file mode 100644 index 0000000000..14fdcb247f --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Transports/TransportTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: ea475033336d48b79b7eb80d51037546 +timeCreated: 1760550018 \ No newline at end of file