diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 3353565e01..a5c1250ccf 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -14,9 +14,9 @@ Additional documentation and release notes are available at [Multiplayer Documen - Added a flag to override command-line arguments (port and ip) in `SetConnectionData`. (#3760) - Added a command-line singleton to parse environment command-line arguments. (#3760) - ### Changed +- Changed when a server is disconnecting a client with a reason it now defers the complete transport disconnect sequence until the end of the frame after the server's transport has sent all pending outbound messages. (#3786) ### Deprecated @@ -27,6 +27,8 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed - Fixed issues with the "Client-server quickstart for Netcode for GameObjects" script having static methods and properties. (#3787) +- Fixed issue where a warning message was being logged upon a client disconnecting from a server when the log level is set to developer. (#3786) +- Fixed issue where the server or host would no longer have access to the transport id to client id table when processing a transport level client disconnect event. (#3786) - Fixed issue where invoking an RPC, on another `NetworkBehaviour` associated with the same `NetworkObject` that is ordered before the `NetworkBehaviour` invoking the RPC, during `OnNetworkSpawn` could throw an exception if scene management is disabled. (#3782) - Fixed issue where the `Axis to Synchronize` toggles didn't work with multi object editing in `NetworkTransform`. (#3781) - Fixed issue where using the dedicated server package would override all attempts to change the port by code. (#3760) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 51f5dc2aeb..29e4be6715 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -362,11 +362,6 @@ internal void RemovePendingClient(ulong 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, false); } @@ -488,6 +483,15 @@ internal void HandleNetworkEvent(NetworkEvent networkEvent, ulong transportClien } } + /// + /// Client's save their assigned transport id. + /// + /// + /// Added to be able to appropriately log the client's transport + /// id when it is shutdown or disconnected. + /// + private ulong m_LocalClientTransportId; + /// /// Handles a event. /// @@ -508,6 +512,8 @@ internal void ConnectEventHandler(ulong transportClientId) } else { + // Cache the local client's transport id. + m_LocalClientTransportId = transportClientId; clientId = NetworkManager.ServerClientId; } @@ -585,9 +591,14 @@ private void GenerateDisconnectInformation(ulong clientId, ulong transportClient /// internal void DisconnectEventHandler(ulong transportClientId) { - var (clientId, wasConnectedClient) = TransportIdCleanUp(transportClientId); - if (!wasConnectedClient) + // Check to see if the client has already been removed from the table but + // do not remove it just yet. + var (clientId, isConnectedClient) = TransportIdToClientId(transportClientId); + + // If the client is not registered and we are the server + if (!isConnectedClient && NetworkManager.IsServer) { + // Then exit early return; } @@ -622,17 +633,12 @@ internal void DisconnectEventHandler(ulong transportClientId) { // We need to process the disconnection before notifying OnClientDisconnectFromServer(clientId); - - // Now notify the client has disconnected - InvokeOnClientDisconnectCallback(clientId); - - if (LocalClient.IsHost) - { - InvokeOnPeerDisconnectedCallback(clientId); - } } else { + // Client's clean up their transport id separately from the server. + TransportIdCleanUp(transportClientId); + // Notify local client of disconnection InvokeOnClientDisconnectCallback(clientId); @@ -793,12 +799,15 @@ private IEnumerator ApprovalTimeout(ulong clientId) /// internal void ApproveConnection(ref ConnectionRequestMessage connectionRequestMessage, ref NetworkContext context) { + if (ConnectionApprovalCallback == null) + { + return; + } // Note: Delegate creation allocates. // Note: ToArray() also allocates. :( var response = new NetworkManager.ConnectionApprovalResponse(); ClientsToApprove[context.SenderId] = response; - - ConnectionApprovalCallback( + ConnectionApprovalCallback?.Invoke( new NetworkManager.ConnectionApprovalRequest { Payload = connectionRequestMessage.ConnectionData, @@ -852,13 +861,6 @@ internal void ProcessPendingApprovals() } } - /// - /// Adding this because message hooks cannot happen fast enough under certain scenarios - /// where the message is sent and responded to before the hook is in place. - /// - internal bool MockSkippingApproval; - - /// /// Server Side: Handles the denial of a client who sent a connection request /// @@ -873,12 +875,36 @@ private void HandleConnectionDisconnect(ulong ownerClientId, string reason = "") { Reason = reason }; - SendMessage(ref disconnectReason, NetworkDelivery.Reliable, ownerClientId); + SendMessage(ref disconnectReason, MessageDeliveryType.DefaultDelivery, ownerClientId); + m_ClientsToDisconnect.Add(ownerClientId); + return; } DisconnectRemoteClient(ownerClientId); } + private List m_ClientsToDisconnect = new List(); + + internal void ProcessClientsToDisconnect() + { + if (m_ClientsToDisconnect.Count == 0) + { + return; + } + foreach (var clientId in m_ClientsToDisconnect) + { + try + { + DisconnectRemoteClient(clientId); + } + catch (Exception ex) + { + Debug.LogException(ex); + } + } + m_ClientsToDisconnect.Clear(); + } + /// /// Server Side: Handles the approval of a client /// @@ -939,12 +965,6 @@ internal void HandleConnectionApproval(ulong ownerClientId, bool createPlayerObj // Server doesn't send itself the connection approved message if (ownerClientId != NetworkManager.ServerClientId) { - if (MockSkippingApproval) - { - NetworkLog.LogInfo("Mocking server not responding with connection approved..."); - return; - } - SendConnectionApprovedMessage(ownerClientId); // If scene management is disabled, then we are done and notify the local host-server the client is connected @@ -1040,7 +1060,7 @@ private void SendConnectionApprovedMessage(ulong approvedClientId) } } - SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, approvedClientId); + SendMessage(ref message, MessageDeliveryType.DefaultDelivery, approvedClientId); message.MessageVersions.Dispose(); message.ConnectedClientIds.Dispose(); @@ -1111,13 +1131,9 @@ internal NetworkClient AddClient(ulong clientId) return ConnectedClients[clientId]; } - var networkClient = LocalClient; - // If this is not the local client then create a new one - if (clientId != NetworkManager.LocalClientId) - { - networkClient = new NetworkClient(); - } + var networkClient = clientId == NetworkManager.LocalClientId ? LocalClient : new NetworkClient(); + networkClient.SetRole(clientId == NetworkManager.ServerClientId, isClient: true, NetworkManager); networkClient.ClientId = clientId; if (!ConnectedClients.ContainsKey(clientId)) @@ -1224,6 +1240,14 @@ internal void OnClientDisconnectFromServer(ulong clientId) // clean up as everything that needs to be destroyed will be during shutdown. if (NetworkManager.ShutdownInProgress && clientId == NetworkManager.ServerClientId) { + // Now notify the client has disconnected. + // (transport id cleanup is handled within) + InvokeOnClientDisconnectCallback(clientId); + + if (LocalClient.IsHost) + { + InvokeOnPeerDisconnectedCallback(clientId); + } return; } @@ -1392,8 +1416,20 @@ internal void OnClientDisconnectFromServer(ulong clientId) } ConnectedClientIds.Remove(clientId); - var message = new ClientDisconnectedMessage { ClientId = clientId }; - MessageManager?.SendMessage(ref message, MessageDeliveryType.DefaultDelivery, ConnectedClientIds); + + if (MessageManager != null) + { + var message = new ClientDisconnectedMessage { ClientId = clientId }; + foreach (var sendToId in ConnectedClientIds) + { + // Do not send a disconnect message to ourself + if (sendToId == NetworkManager.LocalClientId) + { + continue; + } + MessageManager.SendMessage(ref message, MessageDeliveryType.DefaultDelivery, sendToId); + } + } // Used for testing/validation purposes only // Promote a new session owner when the ENABLE_DAHOST_AUTOPROMOTE_SESSION_OWNER scripting define is set @@ -1404,17 +1440,18 @@ internal void OnClientDisconnectFromServer(ulong clientId) var (transportId, idExists) = ClientIdToTransportId(clientId); if (idExists) { - NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId); - - InvokeOnClientDisconnectCallback(clientId); - - if (LocalClient.IsHost) + // Clean up the transport to client (and vice versa) mappings + var (transportIdDisconnected, wasRemoved) = TransportIdCleanUp(transportId); + if (wasRemoved) { - InvokeOnPeerDisconnectedCallback(clientId); - } + NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId); + InvokeOnClientDisconnectCallback(clientId); - // Clean up the transport to client (and vice versa) mappings - TransportIdCleanUp(transportId); + if (LocalClient.IsHost) + { + InvokeOnPeerDisconnectedCallback(clientId); + } + } } // Assure the client id is no longer in the pending clients list @@ -1462,16 +1499,6 @@ internal void DisconnectClient(ulong clientId, string reason = null) return; } - if (!string.IsNullOrEmpty(reason)) - { - var disconnectReason = new DisconnectReasonMessage - { - Reason = reason - }; - SendMessage(ref disconnectReason, NetworkDelivery.Reliable, clientId); - } - - Transport.ClosingRemoteConnection(); var transportId = ClientIdToTransportId(clientId); if (transportId.Item2) { @@ -1491,6 +1518,7 @@ internal void DisconnectClient(ulong clientId, string reason = null) internal void Initialize(NetworkManager networkManager) { // Prepare for a new session + m_LocalClientTransportId = 0; LocalClient.IsApproved = false; m_PendingClients.Clear(); ConnectedClients.Clear(); @@ -1524,8 +1552,9 @@ internal void Shutdown() { Transport.ShuttingDown(); var clientId = NetworkManager ? NetworkManager.LocalClientId : NetworkManager.ServerClientId; - var transportId = ClientIdToTransportId(clientId); - GenerateDisconnectInformation(clientId, transportId.Item1, $"{nameof(NetworkConnectionManager)} was shutdown."); + // Server and host just log 0 for their transport id while clients will log their cached m_LocalClientTransportId + var transportId = clientId == NetworkManager.ServerClientId ? 0 : m_LocalClientTransportId; + GenerateDisconnectInformation(clientId, transportId, $"{nameof(NetworkConnectionManager)} was shutdown."); } if (LocalClient.IsServer) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index 9bed94fa20..dee822ec4f 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -346,7 +346,6 @@ public void NetworkUpdate(NetworkUpdateStage updateStage) AnticipationSystem.SetupForUpdate(); MessageManager.ProcessIncomingMessageQueue(); - MessageManager.CleanupDisconnectedClients(); AnticipationSystem.ProcessReanticipation(); #if COM_UNITY_MODULES_PHYSICS || COM_UNITY_MODULES_PHYSICS2D foreach (var networkObjectEntry in NetworkTransformFixedUpdate) @@ -478,6 +477,18 @@ public void NetworkUpdate(NetworkUpdateStage updateStage) // This is "ok" to invoke when not processing messages since it is just cleaning up messages that never got handled within their timeout period. DeferredMessageManager.CleanupStaleTriggers(); + if (IsServer) + { + // Process any pending clients that need to be disconnected. + // This is typically a disconnect with reason scenario where + // we want the disconnect reason message to be sent prior to + // completely shutting down the endpoint. + ConnectionManager.ProcessClientsToDisconnect(); + } + + // Clean up disconnected clients last + MessageManager.CleanupDisconnectedClients(); + if (m_ShuttingDown) { // Host-server will disconnect any connected clients prior to finalizing its shutdown diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkMessageManager.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkMessageManager.cs index 8401ed147a..c66aa62273 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkMessageManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/NetworkMessageManager.cs @@ -492,6 +492,11 @@ private void CleanupDisconnectedClient(ulong clientId) internal void CleanupDisconnectedClients() { + if (m_DisconnectedClients.Count == 0) + { + return; + } + foreach (var clientId in m_DisconnectedClients) { CleanupDisconnectedClient(clientId); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/ClientOnlyConnectionTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/ClientOnlyConnectionTests.cs index c8677bed40..549f0d8d23 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/ClientOnlyConnectionTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/ClientOnlyConnectionTests.cs @@ -32,9 +32,9 @@ public void Setup() // Default is 1000ms per connection attempt and 60 connection attempts (60s) // Currently there is no easy way to set these values other than in-editor var unityTransport = m_NetworkManagerGameObject.AddComponent(); - unityTransport.ConnectTimeoutMS = 1000; + unityTransport.ConnectTimeoutMS = 500; unityTransport.MaxConnectAttempts = 1; - m_TimeoutHelper = new TimeoutHelper(2); + m_TimeoutHelper = new TimeoutHelper(4); m_ClientNetworkManager.NetworkConfig.NetworkTransport = unityTransport; } @@ -49,7 +49,6 @@ public IEnumerator ClientFailsToConnect() // Unity Transport throws an error when it times out LogAssert.Expect(LogType.Error, "Failed to connect to server."); - yield return NetcodeIntegrationTest.WaitForConditionOrTimeOut(() => m_WasDisconnected, m_TimeoutHelper); Assert.False(m_TimeoutHelper.TimedOut, "Timed out waiting for client to timeout waiting to connect!"); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs index 2c1f412b23..558ec9b9b4 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs @@ -1,7 +1,9 @@ using System.Collections; using System.Collections.Generic; +using System.Text.RegularExpressions; using NUnit.Framework; using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine; using UnityEngine.TestTools; namespace Unity.Netcode.RuntimeTests @@ -28,6 +30,12 @@ public ClientConnectionTests(SceneManagementState sceneManagementState, NetworkT m_SceneManagementEnabled = sceneManagementState == SceneManagementState.SceneManagementEnabled; } + protected override IEnumerator OnSetup() + { + m_ServerCallbackCalled.Clear(); + m_ClientCallbackCalled.Clear(); + return base.OnSetup(); + } protected override void OnServerAndClientsCreated() { m_ServerNetworkManager.NetworkConfig.EnableSceneManagement = m_SceneManagementEnabled; @@ -55,6 +63,27 @@ public IEnumerator VerifyOnClientConnectedCallback() Assert.True(m_ServerCallbackCalled.Count == 1 + NumberOfClients); } + /// + /// Validates that no warnings are logged upon a client disconnecting and the + /// log level is set to developer. + /// + [UnityTest] + public IEnumerator VerifyNoWarningOnClientDisconnect() + { + yield return WaitForConditionOrTimeOut(AllCallbacksCalled); + AssertOnTimeout("Timed out waiting for all clients to be connected!"); + + var authority = GetAuthorityNetworkManager(); + var clientToDisconnect = GetNonAuthorityNetworkManager(); + clientToDisconnect.LogLevel = LogLevel.Developer; + authority.LogLevel = LogLevel.Developer; + + yield return StopOneClient(clientToDisconnect); + + NetcodeLogAssert.LogWasNotReceived(LogType.Warning, new Regex(".*")); + } + + private void Server_OnClientConnectedCallback(ulong clientId) { if (!m_ServerCallbackCalled.Add(clientId)) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs index b630136c61..2e0c81b690 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs @@ -92,17 +92,23 @@ protected override void OnServerAndClientsCreated() private void Client_OnClientDisconnectCallback(ulong clientId) { m_ClientNetworkManagers[0].OnClientDisconnectCallback -= Client_OnClientDisconnectCallback; - m_ClientDisconnectReasonValidated = m_ClientNetworkManagers[0].LocalClientId == clientId && m_ClientNetworkManagers[0].DisconnectReason.Contains(k_InvalidToken); + m_ClientDisconnectReasonValidated = m_ClientNetworkManagers[0].LocalClientId == clientId && m_ClientNetworkManagers[0].ConnectionManager.ServerDisconnectReason.Contains(k_InvalidToken); } - private bool ClientAndHostValidated() + private bool ClientAndHostValidated(StringBuilder errorLog) { if (!m_Validated.ContainsKey(m_ServerNetworkManager.LocalClientId) || !m_Validated[m_ServerNetworkManager.LocalClientId]) { + errorLog.AppendLine($"Server does not contain a validation for Client-{m_ServerNetworkManager.LocalClientId}!"); return false; } if (m_PlayerCreation == PlayerCreation.FailValidation) { + if (!m_ClientDisconnectReasonValidated) + { + errorLog.AppendLine($"{nameof(m_ClientDisconnectReasonValidated)} is false!"); + } + return m_ClientDisconnectReasonValidated; } else @@ -111,6 +117,7 @@ private bool ClientAndHostValidated() { if (!m_Validated.ContainsKey(client.LocalClientId) || !m_Validated[client.LocalClientId]) { + errorLog.AppendLine($"Client-{client.LocalClientId} was not in the validated list!"); return false; } } @@ -163,6 +170,7 @@ private void NetworkManagerObject_ConnectionApprovalCallback(NetworkManager.Conn { response.Approved = false; response.Reason = "Invalid validation token!"; + return; } response.CreatePlayerObject = ShouldCheckForSpawnedPlayers(); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs index db1210face..3b11c5d871 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs @@ -28,7 +28,7 @@ public ConnectionApprovalTimeoutTests(ApprovalTimedOutTypes approvalFailureType) // Must be >= 5 since this is an int value and the test waits for timeout - 1 to try to verify it doesn't // time out early - private const int k_TestTimeoutPeriod = 5; + private const int k_TestTimeoutPeriod = 3; private Regex m_ExpectedLogMessage; private LogType m_LogType; @@ -48,21 +48,47 @@ protected override IEnumerator OnTearDown() protected override void OnServerAndClientsCreated() { - m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod; + // The timeouts (client-server relative) need to be skewed such that either the server or the client will timeout prior to the other timing out. + var serverTimeout = (int)(m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond ? k_TestTimeoutPeriod * 1.5f : k_TestTimeoutPeriod * 0.75f); + var clientTimeout = (int)(m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond ? k_TestTimeoutPeriod * 0.75f : k_TestTimeoutPeriod * 1.5f); + + m_ServerNetworkManager.ConnectionApprovalCallback = ConnectionApproval; + + m_ServerNetworkManager.NetworkConfig.ConnectionApproval = true; + m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = serverTimeout; m_ServerNetworkManager.LogLevel = LogLevel.Developer; - m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod; + + m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = clientTimeout; m_ClientNetworkManagers[0].LogLevel = LogLevel.Developer; + m_ClientNetworkManagers[0].NetworkConfig.ConnectionApproval = true; + base.OnServerAndClientsCreated(); } + private void ConnectionApproval(NetworkManager.ConnectionApprovalRequest connectionApprovalRequest, NetworkManager.ConnectionApprovalResponse connectionApprovalResponse) + { + if (connectionApprovalRequest.ClientNetworkId != NetworkManager.ServerClientId) + { + // For these tests we just always place the newly connecting client into pending + connectionApprovalResponse.Pending = true; + } + else + { + // We always approve the host + connectionApprovalResponse.Approved = true; + } + } + + private const string k_ExpectedLogWhenServerDoesNotRespond = "Timed out waiting for the server to approve the connection request."; + private const string k_ExpectedLogWhenClientDoesNotRequest = "Server detected a transport connection from Client-1, but timed out waiting for the connection request message."; + protected override IEnumerator OnStartedServerAndClients() { if (m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond) { - m_ServerNetworkManager.ConnectionManager.MockSkippingApproval = true; // We catch (don't process) the incoming approval message to simulate the server not sending the approved message in time m_ClientNetworkManagers[0].ConnectionManager.MessageManager.Hook(new MessageCatcher(m_ClientNetworkManagers[0])); - m_ExpectedLogMessage = new Regex("Timed out waiting for the server to approve the connection request."); + m_ExpectedLogMessage = new Regex(k_ExpectedLogWhenServerDoesNotRespond); m_LogType = LogType.Log; } else @@ -72,7 +98,7 @@ protected override IEnumerator OnStartedServerAndClients() m_ServerNetworkManager.ConnectionManager.MessageManager.Hook(new MessageCatcher(m_ServerNetworkManager)); // For this test, we know the timed out client will be Client-1 - m_ExpectedLogMessage = new Regex("Server detected a transport connection from Client-1, but timed out waiting for the connection request message."); + m_ExpectedLogMessage = new Regex(k_ExpectedLogWhenClientDoesNotRequest); m_LogType = LogType.Warning; } yield return null; @@ -87,7 +113,7 @@ public IEnumerator ValidateApprovalTimeout() // Verify we haven't received the time out message yet NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage); - yield return new WaitForSeconds(k_TestTimeoutPeriod * 1.25f); + yield return new WaitForSeconds(k_TestTimeoutPeriod * 1.55f); // We should have the test relative log message by this time. NetcodeLogAssert.LogWasReceived(m_LogType, m_ExpectedLogMessage); @@ -95,8 +121,6 @@ public IEnumerator ValidateApprovalTimeout() VerboseDebug("Checking connected client count"); // It should only have the host client connected Assert.AreEqual(1, m_ServerNetworkManager.ConnectedClients.Count, $"Expected only one client when there were {m_ServerNetworkManager.ConnectedClients.Count} clients connected!"); - - Assert.AreEqual(0, m_ServerNetworkManager.ConnectionManager.PendingClients.Count, $"Expected no pending clients when there were {m_ServerNetworkManager.ConnectionManager.PendingClients.Count} pending clients!"); Assert.True(!m_ClientNetworkManagers[0].LocalClient.IsApproved, $"Expected the client to not have been approved, but it was!"); } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/PeerDisconnectCallbackTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/PeerDisconnectCallbackTests.cs index b7512753a4..a8d0c6a4ef 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/PeerDisconnectCallbackTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/PeerDisconnectCallbackTests.cs @@ -1,6 +1,7 @@ using System.Collections; using System.Collections.Generic; using System.Linq; +using System.Text; using NUnit.Framework; using Unity.Netcode.TestHelpers.Runtime; using UnityEngine.TestTools; @@ -46,16 +47,15 @@ protected override void OnServerAndClientsCreated() // Adjusting client and server timeout periods to reduce test time // Get the tick frequency in milliseconds and triple it for the heartbeat timeout var heartBeatTimeout = (int)(300 * (1.0f / m_ServerNetworkManager.NetworkConfig.TickRate)); - var unityTransport = m_ServerNetworkManager.NetworkConfig.NetworkTransport as Transports.UTP.UnityTransport; - if (unityTransport != null) - { - unityTransport.HeartbeatTimeoutMS = heartBeatTimeout; - } - unityTransport = m_ClientNetworkManagers[0].NetworkConfig.NetworkTransport as Transports.UTP.UnityTransport; - if (unityTransport != null) + foreach (var networkManager in m_NetworkManagers) { - unityTransport.HeartbeatTimeoutMS = heartBeatTimeout; + var unityTransport = networkManager.NetworkConfig.NetworkTransport as Transports.UTP.UnityTransport; + if (unityTransport != null) + { + unityTransport.HeartbeatTimeoutMS = heartBeatTimeout; + } + networkManager.OnConnectionEvent += OnConnectionEventCallback; } base.OnServerAndClientsCreated(); @@ -73,18 +73,29 @@ private void OnConnectionEventCallback(NetworkManager networkManager, Connection switch (data.EventType) { case ConnectionEvent.ClientDisconnected: - Assert.IsFalse(data.PeerClientIds.IsCreated); - ++m_ClientDisconnectCount; - break; + { + Assert.IsFalse(data.PeerClientIds.IsCreated); + if (data.ClientId == m_TargetClientId) + { + ++m_ClientDisconnectCount; + } + break; + } case ConnectionEvent.PeerDisconnected: - Assert.IsFalse(data.PeerClientIds.IsCreated); - ++m_PeerDisconnectCount; - break; + { + if (data.ClientId == m_TargetClientId) + { + Assert.IsFalse(data.PeerClientIds.IsCreated); + ++m_PeerDisconnectCount; + } + break; + } } } private bool m_TargetClientShutdown; private NetworkManager m_TargetClient; + private ulong m_TargetClientId; private void ClientToDisconnect_OnClientStopped(bool wasHost) { @@ -97,10 +108,10 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie { m_TargetClientShutdown = false; m_TargetClient = m_ClientNetworkManagers[disconnectedClient - 1]; + m_TargetClientId = m_TargetClient.LocalClientId; m_TargetClient.OnClientStopped += ClientToDisconnect_OnClientStopped; - foreach (var client in m_ClientNetworkManagers) + foreach (var client in m_NetworkManagers) { - client.OnConnectionEvent += OnConnectionEventCallback; if (m_UseHost) { Assert.IsTrue(client.ConnectedClientsIds.Contains(0ul)); @@ -110,15 +121,6 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie Assert.IsTrue(client.ConnectedClientsIds.Contains(3ul)); Assert.AreEqual(client.ServerIsHost, m_UseHost); } - m_ServerNetworkManager.OnConnectionEvent += OnConnectionEventCallback; - if (m_UseHost) - { - Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(0ul)); - } - Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(1ul)); - Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(2ul)); - Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(3ul)); - Assert.AreEqual(m_ServerNetworkManager.ServerIsHost, m_UseHost); // Set up a WaitForMessageReceived hook. // In some cases the message will be received during StopOneClient, but it is not guaranteed @@ -135,6 +137,7 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie // Used to determine if all clients received the CreateObjectMessage var hooks = new MessageHooksConditional(messageHookEntriesForSpawn); + if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient) { m_ServerNetworkManager.DisconnectClient(disconnectedClient); @@ -151,52 +154,44 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie yield return WaitForConditionOrTimeOut(() => m_TargetClientShutdown); AssertOnTimeout($"Timed out waiting for {m_TargetClient.name} to shutdown!"); - foreach (var client in m_ClientNetworkManagers) + // Check that the client is disconnected and all NetworkManagers have registered this + yield return WaitForConditionOrTimeOut(CheckClientDisconnected); + AssertOnTimeout($"Timed out waiting for {m_TargetClient.name} to register as having shutdown!"); + + // If disconnected, the server and the client that disconnected will be notified + Assert.AreEqual(2, m_ClientDisconnectCount); + // Host receives peer disconnect, dedicated server does not + Assert.AreEqual(m_UseHost ? 3 : 2, m_PeerDisconnectCount); + } + + /// + /// Conditional method to verify the is disconnected + /// and that identifier is not contained on any instance's + /// . + /// + private bool CheckClientDisconnected(StringBuilder errorLog) + { + foreach (var networkManager in m_NetworkManagers) { - if (!client.IsConnectedClient) + if (!networkManager.IsConnectedClient) { - Assert.IsEmpty(client.ConnectedClientsIds); - continue; - } - if (m_UseHost) - { - Assert.IsTrue(client.ConnectedClientsIds.Contains(0ul), $"[Client-{client.LocalClientId}][Connected ({client.IsConnectedClient})] Still has client identifier 0!"); - } - for (var i = 1ul; i < 3ul; ++i) - { - if (i == disconnectedClient) - { - Assert.IsFalse(client.ConnectedClientsIds.Contains(i), $"[Client-{client.LocalClientId}][Connected ({client.IsConnectedClient})] Still has client identifier {i}!"); - } - else - { - Assert.IsTrue(client.ConnectedClientsIds.Contains(i), $"[Client-{client.LocalClientId}][Connected ({client.IsConnectedClient})] Still has client identifier {i}!"); - } + continue; } - } - if (m_UseHost) - { - Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(0ul)); - } - - for (var i = 1ul; i < 3ul; ++i) - { - if (i == disconnectedClient) + if (networkManager.LocalClientId == m_TargetClientId && ((networkManager.IsConnectedClient) || (networkManager.IsListening))) { - Assert.IsFalse(m_ServerNetworkManager.ConnectedClientsIds.Contains(i)); + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Is either still connected or still listening!"); + return false; } - else + if (networkManager.ConnectedClientsIds.Contains(m_TargetClientId)) { - Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(i)); + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Is still has {m_TargetClientId} registered as a connected client!"); + return false; } } - - // If disconnected, the server and the client that disconnected will be notified - Assert.AreEqual(2, m_ClientDisconnectCount); - // Host receives peer disconnect, dedicated server does not - Assert.AreEqual(m_UseHost ? 3 : 2, m_PeerDisconnectCount); + return true; } } } +