From 145b591d2873f7afd8e5ff36c7b8ce81e13aec05 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Fri, 14 Nov 2025 15:53:43 -0600 Subject: [PATCH 1/7] fix This fix prevents the warning message from being logged on the client side when shutting down from the client side and the developer log level is set. This fix also assures that a server or host does not send itself a disconnect message. This fix separated the transport id clean up between client and host/server upon handling a disconnect event which assures the server/host will have access to this transport id when processing a client that disconnected. --- .../Connection/NetworkConnectionManager.cs | 51 ++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 51f5dc2aeb..93fdabcd6b 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,12 @@ 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 (!isConnectedClient) { + // If so, exit early return; } @@ -623,7 +632,8 @@ internal void DisconnectEventHandler(ulong transportClientId) // We need to process the disconnection before notifying OnClientDisconnectFromServer(clientId); - // Now notify the client has disconnected + // Now notify the client has disconnected. + // (transport id cleanup is handled within) InvokeOnClientDisconnectCallback(clientId); if (LocalClient.IsHost) @@ -633,6 +643,9 @@ internal void DisconnectEventHandler(ulong transportClientId) } else { + // Client's clean up their transport id separately from the server. + TransportIdCleanUp(transportClientId); + // Notify local client of disconnection InvokeOnClientDisconnectCallback(clientId); @@ -1392,8 +1405,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 @@ -1491,6 +1516,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 +1550,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) From 63c82c0d1a48dd9d417152b7473ba178c5d567f7 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Fri, 14 Nov 2025 16:33:45 -0600 Subject: [PATCH 2/7] update Adding change log entries. --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 36cfd1f7a2..0ebaffb10b 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -24,6 +24,8 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- 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 the `Axis to Synchronize` toggles didn't work with multi object editing in `NetworkTransform`. (#3781) From cea8998726ab5c52ee5b21d273321dacdf4502fa Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Sun, 16 Nov 2025 13:57:22 -0600 Subject: [PATCH 3/7] test Adding test to check that no warning messages are logged upon a client disconnecting itself from a session. --- .../Connection/ClientConnectionTests.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) 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)) From 610e2d6e3ca8c0d16c60c249997b8a075336381e Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Mon, 17 Nov 2025 18:45:54 -0600 Subject: [PATCH 4/7] refactor and fix Fixing several issues with the disconnect flow. Not removing the transport id until later. The server now only does this within OnClientDisconnectFromServer. When disconnecting a client with a reason, the client is not completely disconnected until the end of the frame when the server has finished sending all pending messages. Fixing some missed MessageDeliveryType replacements. Removing the MockSkippingApproval as it is no longer needed with the fixes in place. --- .../Connection/NetworkConnectionManager.cs | 108 +++++++++--------- .../Runtime/Core/NetworkManager.cs | 13 ++- .../Messaging/NetworkMessageManager.cs | 5 + 3 files changed, 72 insertions(+), 54 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 93fdabcd6b..29e4be6715 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -594,9 +594,11 @@ internal void DisconnectEventHandler(ulong transportClientId) // 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 (!isConnectedClient) + + // If the client is not registered and we are the server + if (!isConnectedClient && NetworkManager.IsServer) { - // If so, exit early + // Then exit early return; } @@ -631,15 +633,6 @@ internal void DisconnectEventHandler(ulong transportClientId) { // We need to process the disconnection before notifying OnClientDisconnectFromServer(clientId); - - // Now notify the client has disconnected. - // (transport id cleanup is handled within) - InvokeOnClientDisconnectCallback(clientId); - - if (LocalClient.IsHost) - { - InvokeOnPeerDisconnectedCallback(clientId); - } } else { @@ -806,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, @@ -865,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 /// @@ -886,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 /// @@ -952,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 @@ -1053,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(); @@ -1124,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)) @@ -1237,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; } @@ -1429,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 @@ -1487,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) { diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index 50a6304f52..0452e7c271 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -348,7 +348,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) @@ -480,6 +479,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); From 0c671420b6c5568c0a9c00acda850b161ffd8b54 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Mon, 17 Nov 2025 18:50:59 -0600 Subject: [PATCH 5/7] test Did a bunch of cleanup on several connection tests to improve stability and use more recent integration test features. ClientOnlyConnectionTests reduced the transport connection timeout and increased the timeout helper. ConnectionApprovalTimeoutTests needed to have the server and client timeout values vary depending upon the test type. PeerDisconnectCallbackTests needed to trap for the peer disconnecting. --- .../Runtime/ClientOnlyConnectionTests.cs | 5 +- .../Tests/Runtime/ConnectionApproval.cs | 12 +- .../Runtime/ConnectionApprovalTimeoutTests.cs | 42 +++++-- .../Runtime/PeerDisconnectCallbackTests.cs | 117 +++++++++--------- 4 files changed, 101 insertions(+), 75 deletions(-) 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/ConnectionApproval.cs b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs index b630136c61..8fecfc9220 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; } } } + From c3d00c131dcf00d392b23627e735b1e26b6f0c0b Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Mon, 17 Nov 2025 18:58:20 -0600 Subject: [PATCH 6/7] style Removing whitespaces --- .../Tests/Runtime/ConnectionApproval.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs index 8fecfc9220..2e0c81b690 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs @@ -108,7 +108,7 @@ private bool ClientAndHostValidated(StringBuilder errorLog) { errorLog.AppendLine($"{nameof(m_ClientDisconnectReasonValidated)} is false!"); } - + return m_ClientDisconnectReasonValidated; } else From 4c0ab7c10a6089708fe232506733ac5aeecd78ab Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Mon, 17 Nov 2025 19:10:44 -0600 Subject: [PATCH 7/7] update adding one more change log entry --- com.unity.netcode.gameobjects/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 36eb32c350..1b90b852df 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### 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