fix: host not generating its own local client disconnect notification (#2822)

* fix

Assure the host generates a notification for the local host client no longer being connected when it shuts itself down.

* test

Include modification to the DisconnectTest to validate this fix.

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.
Adding check for ConnectionEvent.ClientDisconnected for both test types.
Fixing issue with StopOneClient destroying the NetworkManager when destroy is set to false.

Author: NoelStephensUnity

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 `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)
 

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

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);
         }
 

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<NetworkManager, ConnectionEventData> m_DisconnectedEvent = new Dictionary<NetworkManager, ConnectionEventData>();
+        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);
+        }
+
         /// <summary>
         /// Conditional check to assure the transport to client (and vice versa) mappings are cleaned up
         /// </summary>
@@ -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,18 +149,39 @@ 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]);
             }
 
             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
@@ -161,6 +197,21 @@ 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_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!");
+            }
         }
     }
 }

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<ulong> m_ClientsDisconnected = new List<ulong>();
+        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!");
         }
     }
 }