From 6e9afdbf509cb7d7c37bca9102ea865c05d40780 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Sun, 4 Feb 2024 17:41:54 -0600 Subject: [PATCH 1/8] fix Assure the host generates a notification for the local host client no longer being connected when it shuts itself down. --- .../Runtime/Core/NetworkManager.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index 4fd66e3bfb..b761d69d68 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -1185,6 +1185,12 @@ internal void ShutdownInternal() IsListening = false; m_ShuttingDown = false; + // Generate a local notification that the host client is disconnected + if (IsHost) + { + ConnectionManager.InvokeOnClientDisconnectCallback(LocalClientId); + } + if (ConnectionManager.LocalClient.IsClient) { // If we were a client, we want to know if we were a host From 7c0ef24892e286231d169013bb41941bff62089a Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Sun, 4 Feb 2024 17:51:38 -0600 Subject: [PATCH 2/8] test Include modification to the DisconnectTest to validate this fix. --- .../Tests/Runtime/DisconnectTests.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs index c72cfdd4c7..7c3387482a 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs @@ -161,6 +161,17 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client yield return WaitForConditionOrTimeOut(TransportIdCleanedUp); AssertOnTimeout("Timed out waiting for transport and client id mappings to be cleaned up!"); + + // Validate the host-client generates a OnClientDisconnected event when it shutsdown. + // Only test when the test run is the client disconnecting from the server (otherwise the server will be shutdown already) + if (clientDisconnectType == ClientDisconnectType.ClientDisconnectsFromServer) + { + m_ClientDisconnected = false; + m_ServerNetworkManager.Shutdown(); + + yield return WaitForConditionOrTimeOut(() => m_ClientDisconnected); + AssertOnTimeout("Timed out waiting for host-client to generate disconnect message!"); + } } } } From 31fa58f7f04dd2bd93a78bb40546df2dd67fa260 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Sun, 4 Feb 2024 17:55:48 -0600 Subject: [PATCH 3/8] update updating the change log --- 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 233a89f835..41392cc4d0 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed issue where the host was not invoking OnClientDisconnected for its own local client when internally shutting down. (#2822) - Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807) - Fixed issue where in-scene placed `NetworkObject`s with complex nested children `NetworkObject`s (more than one child in depth) would not synchronize properly if WorldPositionStays was set to true. (#2796) From d2cdfb8ea131ca8a86a2d2f3752a01a87e5b67e1 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Sun, 4 Feb 2024 17:59:45 -0600 Subject: [PATCH 4/8] update fixing improper name for callback --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 41392cc4d0..8c3efc4b66 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,7 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed -- Fixed issue where the host was not invoking OnClientDisconnected for its own local client when internally shutting down. (#2822) +- Fixed issue where the host was not invoking OnClientDisconnectCallback for its own local client when internally shutting down. (#2822) - Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807) - Fixed issue where in-scene placed `NetworkObject`s with complex nested children `NetworkObject`s (more than one child in depth) would not synchronize properly if WorldPositionStays was set to true. (#2796) From 9f5ce0b23ab8f2348dde491404d5b34e3e49563e Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Sun, 4 Feb 2024 18:00:36 -0600 Subject: [PATCH 5/8] style --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 8c3efc4b66..65ebca9bf7 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,7 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed -- Fixed issue where the host was not invoking OnClientDisconnectCallback for its own local client when internally shutting down. (#2822) +- Fixed issue where the host was not invoking `OnClientDisconnectCallback` for its own local client when internally shutting down. (#2822) - Fixed issue where NetworkTransform could potentially attempt to "unregister" a named message prior to it being registered. (#2807) - Fixed issue where in-scene placed `NetworkObject`s with complex nested children `NetworkObject`s (more than one child in depth) would not synchronize properly if WorldPositionStays was set to true. (#2796) From 2470bb437072702a3d95cfdd9939b3962b6e12cd Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Mon, 5 Feb 2024 08:13:13 -0600 Subject: [PATCH 6/8] test Rewrote the SenderIdTest to be a bit more flexible with regard to waiting for the client disconnect event and not failing if it received anything afterwards. --- .../Assets/Tests/Runtime/SenderIdTests.cs | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/testproject/Assets/Tests/Runtime/SenderIdTests.cs b/testproject/Assets/Tests/Runtime/SenderIdTests.cs index 54109383a3..a7c2faa9f0 100644 --- a/testproject/Assets/Tests/Runtime/SenderIdTests.cs +++ b/testproject/Assets/Tests/Runtime/SenderIdTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections; +using System.Collections.Generic; using NUnit.Framework; using Unity.Collections; using Unity.Netcode; @@ -89,36 +90,34 @@ public IEnumerator WhenSendingMessageFromClientToServer_SenderIdIsCorrect() Assert.IsTrue(secondReceived); } + + private List m_ClientsDisconnected = new List(); + private ulong m_ClientToValidateDisconnected; + + private bool ValidateClientId() + { + return m_ClientsDisconnected.Contains(m_ClientToValidateDisconnected); + } + + private void OnClientDisconnected(ulong clientId) + { + m_ClientsDisconnected.Add(clientId); + } + [UnityTest] public IEnumerator WhenClientDisconnectsFromServer_ClientIdIsCorrect() { - var firstClientId = FirstClient.LocalClientId; - bool received = false; - void firstCallback(ulong id) - { - Assert.AreEqual(firstClientId, id); - received = true; - } - m_ServerNetworkManager.OnClientDisconnectCallback += firstCallback; + m_ClientsDisconnected.Clear(); + m_ServerNetworkManager.OnClientDisconnectCallback += OnClientDisconnected; + m_ClientToValidateDisconnected = m_ClientNetworkManagers[0].LocalClientId; FirstClient.Shutdown(); + yield return WaitForConditionOrTimeOut(ValidateClientId); + AssertOnTimeout($"Timed out waiting for the server to receive Client-{m_ClientToValidateDisconnected} disconnect event!"); - yield return new WaitForSeconds(0.2f); - - Assert.IsTrue(received); - var secondClientId = SecondClient.LocalClientId; - received = false; - - m_ServerNetworkManager.OnClientDisconnectCallback -= firstCallback; - m_ServerNetworkManager.OnClientDisconnectCallback += id => - { - Assert.AreEqual(secondClientId, id); - received = true; - }; + m_ClientToValidateDisconnected = m_ClientNetworkManagers[1].LocalClientId; SecondClient.Shutdown(); - - yield return new WaitForSeconds(0.2f); - - Assert.IsTrue(received); + yield return WaitForConditionOrTimeOut(ValidateClientId); + AssertOnTimeout($"Timed out waiting for the server to receive Client-{m_ClientToValidateDisconnected} disconnect event!"); } } } From 49194e0952e6b442e6e7d3fb03b9d980f8251c06 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Mon, 5 Feb 2024 08:36:05 -0600 Subject: [PATCH 7/8] style whitespace removal --- testproject/Assets/Tests/Runtime/SenderIdTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testproject/Assets/Tests/Runtime/SenderIdTests.cs b/testproject/Assets/Tests/Runtime/SenderIdTests.cs index a7c2faa9f0..92ebf2f4a1 100644 --- a/testproject/Assets/Tests/Runtime/SenderIdTests.cs +++ b/testproject/Assets/Tests/Runtime/SenderIdTests.cs @@ -98,7 +98,7 @@ private bool ValidateClientId() { return m_ClientsDisconnected.Contains(m_ClientToValidateDisconnected); } - + private void OnClientDisconnected(ulong clientId) { m_ClientsDisconnected.Add(clientId); From 67c9e8070d88b05a667ce16b7963fbe5f9630d55 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Mon, 5 Feb 2024 19:25:22 -0600 Subject: [PATCH 8/8] test - fix Adding check for ConnectionEvent.ClientDisconnected for both test types. Fixing issue with StopOneClient destroying the NetworkManager when destroy is set to false. --- .../Runtime/NetcodeIntegrationTest.cs | 5 ++- .../Tests/Runtime/DisconnectTests.cs | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs index f05ee46cfe..1c8b4c6eab 100644 --- a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs +++ b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs @@ -534,7 +534,10 @@ protected void CreateAndStartNewClientWithTimeTravel() protected IEnumerator StopOneClient(NetworkManager networkManager, bool destroy = false) { NetcodeIntegrationTestHelpers.StopOneClient(networkManager, destroy); - AddRemoveNetworkManager(networkManager, false); + if (destroy) + { + AddRemoveNetworkManager(networkManager, false); + } yield return WaitForConditionOrTimeOut(() => !networkManager.IsConnectedClient); } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs index 7c3387482a..bdc298a6a6 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/DisconnectTests.cs @@ -1,4 +1,5 @@ using System.Collections; +using System.Collections.Generic; using System.Linq; using NUnit.Framework; using Unity.Netcode.TestHelpers.Runtime; @@ -37,7 +38,10 @@ public enum ClientDisconnectType protected override int NumberOfClients => 1; private OwnerPersistence m_OwnerPersistence; + private ClientDisconnectType m_ClientDisconnectType; private bool m_ClientDisconnected; + private Dictionary m_DisconnectedEvent = new Dictionary(); + private ulong m_DisconnectEventClientId; private ulong m_TransportClientId; private ulong m_ClientId; @@ -89,6 +93,16 @@ private void OnClientDisconnectCallback(ulong obj) m_ClientDisconnected = true; } + private void OnConnectionEvent(NetworkManager networkManager, ConnectionEventData connectionEventData) + { + if (connectionEventData.EventType != ConnectionEvent.ClientDisconnected) + { + return; + } + + m_DisconnectedEvent.Add(networkManager, connectionEventData); + } + /// /// Conditional check to assure the transport to client (and vice versa) mappings are cleaned up /// @@ -126,6 +140,7 @@ private bool DoesServerStillHaveSpawnedPlayerObject() public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType clientDisconnectType) { m_ClientId = m_ClientNetworkManagers[0].LocalClientId; + m_ClientDisconnectType = clientDisconnectType; var serverSideClientPlayer = m_ServerNetworkManager.ConnectionManager.ConnectedClients[m_ClientId].PlayerObject; @@ -134,11 +149,15 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient) { m_ClientNetworkManagers[0].OnClientDisconnectCallback += OnClientDisconnectCallback; + m_ClientNetworkManagers[0].OnConnectionEvent += OnConnectionEvent; + m_ServerNetworkManager.OnConnectionEvent += OnConnectionEvent; m_ServerNetworkManager.DisconnectClient(m_ClientId); } else { m_ServerNetworkManager.OnClientDisconnectCallback += OnClientDisconnectCallback; + m_ServerNetworkManager.OnConnectionEvent += OnConnectionEvent; + m_ClientNetworkManagers[0].OnConnectionEvent += OnConnectionEvent; yield return StopOneClient(m_ClientNetworkManagers[0]); } @@ -146,6 +165,23 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client yield return WaitForConditionOrTimeOut(() => m_ClientDisconnected); AssertOnTimeout("Timed out waiting for client to disconnect!"); + if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient) + { + Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ServerNetworkManager), $"Could not find the server {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ClientNetworkManagers[0]), $"Could not find the client {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent[m_ClientNetworkManagers[0]].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the client {nameof(NetworkManager)} disconnect event entry!"); + // Unregister for this event otherwise it will be invoked during teardown + m_ServerNetworkManager.OnConnectionEvent -= OnConnectionEvent; + } + else + { + Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ServerNetworkManager), $"Could not find the server {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ClientNetworkManagers[0]), $"Could not find the client {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent[m_ClientNetworkManagers[0]].ClientId == m_ClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the client {nameof(NetworkManager)} disconnect event entry!"); + } + if (m_OwnerPersistence == OwnerPersistence.DestroyWithOwner) { // When we are destroying with the owner, validate the player object is destroyed on the server side @@ -166,11 +202,15 @@ public IEnumerator ClientPlayerDisconnected([Values] ClientDisconnectType client // Only test when the test run is the client disconnecting from the server (otherwise the server will be shutdown already) if (clientDisconnectType == ClientDisconnectType.ClientDisconnectsFromServer) { + m_DisconnectedEvent.Clear(); m_ClientDisconnected = false; m_ServerNetworkManager.Shutdown(); yield return WaitForConditionOrTimeOut(() => m_ClientDisconnected); AssertOnTimeout("Timed out waiting for host-client to generate disconnect message!"); + + Assert.IsTrue(m_DisconnectedEvent.ContainsKey(m_ServerNetworkManager), $"Could not find the server {nameof(NetworkManager)} disconnect event entry!"); + Assert.IsTrue(m_DisconnectedEvent[m_ServerNetworkManager].ClientId == NetworkManager.ServerClientId, $"Expected ClientID {m_ClientId} but found ClientID {m_DisconnectedEvent[m_ServerNetworkManager].ClientId} for the server {nameof(NetworkManager)} disconnect event entry!"); } } }