fix: client throws a not server exception when destroying a NetworkObject while shutting down [MTT-6210]

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where a client could throw an exception if abruptly disconnected from a network session with one or more spawned `NetworkObject`(s). (#2510)
 - Fixed issue where invalid endpoint addresses were not being detected and returning false from NGO UnityTransport. (#2496)
 - Fixed some errors that could occur if a connection is lost and the loss is detected when attempting to write to the socket. (#2495)
 

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -531,14 +531,30 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
 
         private void OnDestroy()
         {
-            if (NetworkManager != null && NetworkManager.IsListening && NetworkManager.IsServer == false && IsSpawned &&
+            // If no NetworkManager is assigned, then just exit early
+            if (!NetworkManager)
+            {
+                return;
+            }
+
+            if (NetworkManager.IsListening && !NetworkManager.IsServer && IsSpawned &&
                 (IsSceneObject == null || (IsSceneObject.Value != true)))
             {
-                throw new NotServerException($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
+                // Clients should not despawn NetworkObjects while connected to a session, but we don't want to destroy the current call stack
+                // if this happens. Instead, we should just generate a network log error and exit early (as long as we are not shutting down).
+                if (!NetworkManager.ShutdownInProgress)
+                {
+                    // Since we still have a session connection, log locally and on the server to inform user of this issue.
+                    if (NetworkManager.LogLevel <= LogLevel.Error)
+                    {
+                        NetworkLog.LogErrorServer($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
+                    }
+                    return;
+                }
+                // Otherwise, clients can despawn NetworkObjects while shutting down and should not generate any messages when this happens
             }
 
-            if (NetworkManager != null && NetworkManager.SpawnManager != null &&
-                NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
+            if (NetworkManager.SpawnManager != null && NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
             {
                 if (this == networkObject)
                 {

com.unity.netcode.gameobjects/Runtime/Logging/NetworkLog.cs

@@ -51,35 +51,58 @@ public static class NetworkLog
         /// <param name="message">The message to log</param>
         public static void LogErrorServer(string message) => LogServer(message, LogType.Error);
 
+        internal static NetworkManager NetworkManagerOverride;
+
         private static void LogServer(string message, LogType logType)
         {
+            var networkManager = NetworkManagerOverride ??= NetworkManager.Singleton;
             // Get the sender of the local log
-            ulong localId = NetworkManager.Singleton != null ? NetworkManager.Singleton.LocalClientId : 0;
-
+            ulong localId = networkManager?.LocalClientId ?? 0;
+            bool isServer = networkManager?.IsServer ?? true;
             switch (logType)
             {
                 case LogType.Info:
-                    LogInfoServerLocal(message, localId);
+                    if (isServer)
+                    {
+                        LogInfoServerLocal(message, localId);
+                    }
+                    else
+                    {
+                        LogInfo(message);
+                    }
                     break;
                 case LogType.Warning:
-                    LogWarningServerLocal(message, localId);
+                    if (isServer)
+                    {
+                        LogWarningServerLocal(message, localId);
+                    }
+                    else
+                    {
+                        LogWarning(message);
+                    }
                     break;
                 case LogType.Error:
-                    LogErrorServerLocal(message, localId);
+                    if (isServer)
+                    {
+                        LogErrorServerLocal(message, localId);
+                    }
+                    else
+                    {
+                        LogError(message);
+                    }
                     break;
             }
 
-            if (NetworkManager.Singleton != null && !NetworkManager.Singleton.IsServer && NetworkManager.Singleton.NetworkConfig.EnableNetworkLogs)
+            if (!isServer && networkManager.NetworkConfig.EnableNetworkLogs)
             {
-
                 var networkMessage = new ServerLogMessage
                 {
                     LogType = logType,
                     Message = message
                 };
-                var size = NetworkManager.Singleton.ConnectionManager.SendMessage(ref networkMessage, NetworkDelivery.ReliableFragmentedSequenced, NetworkManager.ServerClientId);
+                var size = networkManager.ConnectionManager.SendMessage(ref networkMessage, NetworkDelivery.ReliableFragmentedSequenced, NetworkManager.ServerClientId);
 
-                NetworkManager.Singleton.NetworkMetrics.TrackServerLogSent(NetworkManager.ServerClientId, (uint)logType, size);
+                networkManager.NetworkMetrics.TrackServerLogSent(NetworkManager.ServerClientId, (uint)logType, size);
             }
         }
 

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeLogAssert.cs

@@ -147,6 +147,7 @@ public void LogWasReceived(LogType type, Regex messageRegex)
                     if (logEvent.LogType == type && messageRegex.IsMatch(logEvent.Message))
                     {
                         found = true;
+                        break;
                     }
                 }
 
@@ -157,6 +158,23 @@ public void LogWasReceived(LogType type, Regex messageRegex)
             }
         }
 
+        public bool HasLogBeenReceived(LogType type, string message)
+        {
+            var found = false;
+            lock (m_Lock)
+            {
+                foreach (var logEvent in AllLogs)
+                {
+                    if (logEvent.LogType == type && message.Equals(logEvent.Message))
+                    {
+                        found = true;
+                        break;
+                    }
+                }
+            }
+            return found;
+        }
+
         public void Reset()
         {
             lock (m_Lock)

com.unity.netcode.gameobjects/Tests/Runtime/Metrics/ServerLogsMetricTests.cs

@@ -37,6 +37,8 @@ private int GetWriteSizeForLog(NetworkLog.LogType logType, string logMessage)
         [UnityTest]
         public IEnumerator TrackServerLogSentMetric()
         {
+            // Set the client NetworkManager to assure the log is sent
+            NetworkLog.NetworkManagerOverride = Client;
             var waitForSentMetric = new WaitForEventMetricValues<ServerLogEvent>(ClientMetrics.Dispatcher, NetworkMetricTypes.ServerLogSent);
 
             var message = Guid.NewGuid().ToString();
@@ -82,6 +84,12 @@ public IEnumerator TrackServerLogReceivedMetric()
             var serializedLength = GetWriteSizeForLog(NetworkLog.LogType.Warning, message);
             Assert.AreEqual(serializedLength, receivedMetric.BytesCount);
         }
+
+        protected override IEnumerator OnTearDown()
+        {
+            NetworkLog.NetworkManagerOverride = null;
+            return base.OnTearDown();
+        }
     }
 }
 #endif

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDestroyTests.cs

@@ -52,24 +52,63 @@ public IEnumerator TestNetworkObjectServerDestroy()
             Assert.IsTrue(go == null);
         }
 
+
+        public enum ClientDestroyObject
+        {
+            ShuttingDown,
+            ActiveSession
+        }
         /// <summary>
-        /// Tests that a client cannot destroy a spawned networkobject.
+        /// Validates the expected behavior when the client-side destroys a <see cref="NetworkObject"/>
         /// </summary>
-        /// <returns></returns>
         [UnityTest]
-        public IEnumerator TestNetworkObjectClientDestroy()
+        public IEnumerator TestNetworkObjectClientDestroy([Values] ClientDestroyObject clientDestroyObject)
         {
-            // This is the *SERVER VERSION* of the *CLIENT PLAYER*
-            var serverClientPlayerResult = new NetcodeIntegrationTestHelpers.ResultWrapper<NetworkObject>();
-            yield return NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentation(x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, m_ServerNetworkManager, serverClientPlayerResult);
+            var isShuttingDown = clientDestroyObject == ClientDestroyObject.ShuttingDown;
+            var clientPlayer = m_ClientNetworkManagers[0].LocalClient.PlayerObject;
+            var clientId = clientPlayer.OwnerClientId;
 
-            // This is the *CLIENT VERSION* of the *CLIENT PLAYER*
-            var clientClientPlayerResult = new NetcodeIntegrationTestHelpers.ResultWrapper<NetworkObject>();
-            yield return NetcodeIntegrationTestHelpers.GetNetworkObjectByRepresentation(x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId, m_ClientNetworkManagers[0], clientClientPlayerResult);
+            //destroying a NetworkObject while shutting down is allowed
+            if (isShuttingDown)
+            {
+                m_ClientNetworkManagers[0].Shutdown();
+            }
+            else
+            {
+                LogAssert.ignoreFailingMessages = true;
+                NetworkLog.NetworkManagerOverride = m_ClientNetworkManagers[0];
+            }
 
-            // destroy the client player, this is not allowed
-            LogAssert.Expect(LogType.Exception, "NotServerException: Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead.");
-            Object.DestroyImmediate(clientClientPlayerResult.Result.gameObject);
+            Object.DestroyImmediate(clientPlayer.gameObject);
+
+            // destroying a NetworkObject while a session is active is not allowed
+            if (!isShuttingDown)
+            {
+                yield return WaitForConditionOrTimeOut(HaveLogsBeenReceived);
+                AssertOnTimeout($"Not all expected logs were received when destroying a {nameof(NetworkObject)} on the client side during an active session!");
+            }
+        }
+
+        private bool HaveLogsBeenReceived()
+        {
+            if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, "[Netcode] Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
+            {
+                return false;
+            }
+
+            if (!NetcodeLogAssert.HasLogBeenReceived(LogType.Error, $"[Netcode-Server Sender={m_ClientNetworkManagers[0].LocalClientId}] Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead."))
+            {
+                return false;
+            }
+
+            return true;
+        }
+
+        protected override IEnumerator OnTearDown()
+        {
+            NetworkLog.NetworkManagerOverride = null;
+            LogAssert.ignoreFailingMessages = false;
+            return base.OnTearDown();
         }
     }
 }