fix: shutdown and shutdown notification issues [MTT-7791] [MTT-7540] (#2789)

* fix

Just return from OnClientDisconnectFromServer when the server is shutting down as there is no point in doing ownership cleanup or client disconnected notification messages at that point.

* Fix

Approaching this fix a different way since we still should send out client disconnect notifications and such.

* fix

Server or host was not sending disconnect when it shutdown.

* update

When a client disconnects itself or is disconnected by the server, it now returns its client identifier.

Server or host shutdown sequence now includes a "soft shutdown" where the server-host will send out disconnection messages to all clients with the reason for the client being disconnected.

* update

the test project exit button script is now a bit more intelligent with how it handles exiting and now logs the reason for disconnection.

* fix - testproject

fixing exceptions when certain missing properties are not set (primarily buttons or toggles)

* test

updating tests for changes.

* fix

Don't allow a host or server to disconnect its local client.

* update

Adding changelog entries

* style

removing unused namespace.

* fix

Fixes issue with NetworkVariable and NetworkList throwing exceptions upon shutdown.

* Test

Validates that setting NetworkVariables or NetworkList values during OnNetworkDespawn does not throw an exception.

* update

adding change log entry

* style

fixing warning message about setting network variable/list values during a shutdown.

* test

Removing unrequired null check

* style

remove the cr/lf after some else if statments

* Minor formatting changes.

* update

Adjusting warning message language.
Adjusting when the send queue is processed.

* update

reverting back moving the MessageManager?.ProcessSendQueues(); within Shutdown and removing the migration of that out of OnClientDisconnectFromServer.

---------

Co-authored-by: Kitty Draper <[email protected]>

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -21,14 +21,22 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where a client disconnected by a server-host would not receive a local notification. (#2789)
+- Fixed issue where a server-host could shutdown during a relay connection but periodically the transport disconnect message sent to any connected clients could be dropped. (#2789)
+- Fixed issue where a host could disconnect its local client but remain running as a server. (#2789)
+- Fixed issue where `OnClientDisconnectedCallback` was not being invoked under certain conditions. (#2789)
+- Fixed issue where `OnClientDisconnectedCallback` was always returning 0 as the client identifier. (#2789)
+- Fixed issue where if a host or server shutdown while a client owned NetworkObjects (other than the player) it would throw an exception. (#2789)
+- Fixed issue where setting values on a `NetworkVariable` or `NetworkList` within `OnNetworkDespawn` during a shutdown sequence would throw an exception. (#2789)
 - Fixed issue where a teleport state could potentially be overridden by a previous unreliable delta state. (#2777)
 - Fixed issue where `NetworkTransform` was using the `NetworkManager.ServerTime.Tick` as opposed to `NetworkManager.NetworkTickSystem.ServerTime.Tick` during the authoritative side's tick update where it performed a delta state check. (#2777)
 - Fixed issue where a parented in-scene placed NetworkObject would be destroyed upon a client or server exiting a network session but not unloading the original scene in which the NetworkObject was placed. (#2737)
 - Fixed issue where during client synchronization and scene loading, when client synchronization or the scene loading mode are set to `LoadSceneMode.Single`, a `CreateObjectMessage` could be received, processed, and the resultant spawned `NetworkObject` could be instantiated in the client's currently active scene that could, towards the end of the client synchronization or loading process, be unloaded and cause the newly created `NetworkObject` to be destroyed (and throw and exception). (#2735)
 - Fixed issue where a `NetworkTransform` instance with interpolation enabled would result in wide visual motion gaps (stuttering) under above normal latency conditions and a 1-5% or higher packet are drop rate. (#2713)
 
 ### Changed
-
+- Changed the server or host shutdown so it will now perform a "soft shutdown" when `NetworkManager.Shutdown` is invoked. This will send a disconnect notification to all connected clients and the server-host will wait for all connected clients to disconnect or timeout after a 5 second period before completing the shutdown process. (#2789)
+- Changed `OnClientDisconnectedCallback` will now return the assigned client identifier on the local client side if the client was approved and assigned one prior to being disconnected. (#2789)
 - Changed `NetworkTransform.SetState` (and related methods) now are cumulative during a fractional tick period and sent on the next pending tick. (#2777)
 - `NetworkManager.ConnectedClientsIds` is now accessible on the client side and will contain the list of all clients in the session, including the host client if the server is operating in host mode (#2762)
 - Changed `NetworkSceneManager` to return a `SceneEventProgress` status and not throw exceptions for methods invoked when scene management is disabled and when a client attempts to access a `NetworkSceneManager` method by a client. (#2735)

com.unity.netcode.gameobjects/Components/NetworkAnimator.cs

@@ -833,8 +833,7 @@ private void CheckForStateChange(int layer)
                     stateChangeDetected = true;
                     //Debug.Log($"[Cross-Fade] To-Hash: {nt.fullPathHash} | TI-Duration: ({tt.duration}) | TI-Norm: ({tt.normalizedTime}) | From-Hash: ({m_AnimationHash[layer]}) | SI-FPHash: ({st.fullPathHash}) | SI-Norm: ({st.normalizedTime})");
                 }
-                else
-                if (!tt.anyState && tt.fullPathHash != m_TransitionHash[layer])
+                else if (!tt.anyState && tt.fullPathHash != m_TransitionHash[layer])
                 {
                     // first time in this transition for this layer
                     m_TransitionHash[layer] = tt.fullPathHash;
@@ -1053,8 +1052,7 @@ private unsafe void WriteParameters(ref FastBufferWriter writer)
                         BytePacker.WriteValuePacked(writer, valueBool);
                     }
                 }
-                else
-                if (cacheValue.Type == AnimationParamEnumWrapper.AnimatorControllerParameterFloat)
+                else if (cacheValue.Type == AnimationParamEnumWrapper.AnimatorControllerParameterFloat)
                 {
                     var valueFloat = m_Animator.GetFloat(hash);
                     fixed (void* value = cacheValue.Value)

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

@@ -347,7 +347,7 @@ internal ulong TransportIdCleanUp(ulong transportId)
             // When this happens, the client will not have an entry within the m_TransportIdToClientIdMap or m_ClientIdToTransportIdMap lookup tables so we exit early and just return 0 to be used for the disconnect event.
             if (!LocalClient.IsServer && !TransportIdToClientIdMap.ContainsKey(transportId))
             {
-                return 0;
+                return NetworkManager.LocalClientId;
             }
 
             var clientId = TransportIdToClientId(transportId);
@@ -476,12 +476,18 @@ internal void DisconnectEventHandler(ulong transportClientId)
             s_TransportDisconnect.Begin();
 #endif
             var clientId = TransportIdCleanUp(transportClientId);
-
             if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
             {
                 NetworkLog.LogInfo($"Disconnect Event From {clientId}");
             }
 
+            // If we are a client and we have gotten the ServerClientId back, then use our assigned local id as the client that was
+            // disconnected (either the user disconnected or the server disconnected, but the client that disconnected is the LocalClientId)
+            if (!NetworkManager.IsServer && clientId == NetworkManager.ServerClientId)
+            {
+                clientId = NetworkManager.LocalClientId;
+            }
+
             // Process the incoming message queue so that we get everything from the server disconnecting us or, if we are the server, so we got everything from that client.
             MessageManager.ProcessIncomingMessageQueue();
 
@@ -496,9 +502,12 @@ internal void DisconnectEventHandler(ulong transportClientId)
             {
                 OnClientDisconnectFromServer(clientId);
             }
-            else
+            else // As long as we are not in the middle of a shutdown
+            if (!NetworkManager.ShutdownInProgress)
             {
-                // We must pass true here and not process any sends messages as we are no longer connected and thus there is no one to send any messages to and this will cause an exception within UnityTransport as the client ID is no longer valid.
+                // We must pass true here and not process any sends messages as we are no longer connected.
+                // Otherwise, attempting to process messages here can cause an exception within UnityTransport
+                // as the client ID is no longer valid.
                 NetworkManager.Shutdown(true);
             }
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
@@ -901,6 +910,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
 
             // If we are shutting down and this is the server or host disconnecting, then ignore
             // clean up as everything that needs to be destroyed will be during shutdown.
+
             if (NetworkManager.ShutdownInProgress && clientId == NetworkManager.ServerClientId)
             {
                 return;
@@ -924,7 +934,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                             NetworkManager.SpawnManager.DespawnObject(playerObject, true);
                         }
                     }
-                    else
+                    else if (!NetworkManager.ShutdownInProgress)
                     {
                         playerObject.RemoveOwnership();
                     }
@@ -943,7 +953,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                 }
                 else
                 {
-                    // Handle changing ownership and prefab handlers                    
+                    // Handle changing ownership and prefab handlers
                     for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
                     {
                         var ownedObject = clientOwnedObjects[i];
@@ -960,7 +970,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                                     Object.Destroy(ownedObject.gameObject);
                                 }
                             }
-                            else
+                            else if (!NetworkManager.ShutdownInProgress)
                             {
                                 ownedObject.RemoveOwnership();
                             }
@@ -982,7 +992,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
 
                 ConnectedClientIds.Remove(clientId);
                 var message = new ClientDisconnectedMessage { ClientId = clientId };
-                NetworkManager.MessageManager.SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, ConnectedClientIds);
+                MessageManager?.SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, ConnectedClientIds);
             }
 
             // If the client ID transport map exists
@@ -1033,6 +1043,12 @@ internal void DisconnectClient(ulong clientId, string reason = null)
                 throw new NotServerException($"Only server can disconnect remote clients. Please use `{nameof(Shutdown)}()` instead.");
             }
 
+            if (clientId == NetworkManager.ServerClientId)
+            {
+                Debug.LogWarning($"Disconnecting the local server-host client is not allowed. Use NetworkManager.Shutdown instead.");
+                return;
+            }
+
             if (!string.IsNullOrEmpty(reason))
             {
                 var disconnectReason = new DisconnectReasonMessage
@@ -1077,16 +1093,8 @@ internal void Initialize(NetworkManager networkManager)
         /// </summary>
         internal void Shutdown()
         {
-            LocalClient.IsApproved = false;
-            LocalClient.IsConnected = false;
-            ConnectedClients.Clear();
-            ConnectedClientIds.Clear();
-            ConnectedClientsList.Clear();
             if (LocalClient.IsServer)
             {
-                // make sure all messages are flushed before transport disconnect clients
-                MessageManager?.ProcessSendQueues();
-
                 // Build a list of all client ids to be disconnected
                 var disconnectedIds = new HashSet<ulong>();
 
@@ -1122,9 +1130,15 @@ internal void Shutdown()
                 {
                     DisconnectRemoteClient(clientId);
                 }
+
+                // make sure all messages are flushed before transport disconnects clients
+                MessageManager?.ProcessSendQueues();
             }
             else if (NetworkManager != null && NetworkManager.IsListening && LocalClient.IsClient)
             {
+                // make sure all messages are flushed before disconnecting
+                MessageManager?.ProcessSendQueues();
+
                 // Client only, send disconnect and if transport throws and exception, log the exception and continue the shutdown sequence (or forever be shutting down)
                 try
                 {
@@ -1136,6 +1150,12 @@ internal void Shutdown()
                 }
             }
 
+            LocalClient.IsApproved = false;
+            LocalClient.IsConnected = false;
+            ConnectedClients.Clear();
+            ConnectedClientIds.Clear();
+            ConnectedClientsList.Clear();
+
             if (NetworkManager != null && NetworkManager.NetworkConfig?.NetworkTransport != null)
             {
                 NetworkManager.NetworkConfig.NetworkTransport.OnTransportEvent -= HandleNetworkEvent;

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

@@ -73,13 +73,89 @@ public void NetworkUpdate(NetworkUpdateStage updateStage)
 
                         if (m_ShuttingDown)
                         {
-                            ShutdownInternal();
+                            // Host-server will disconnect any connected clients prior to finalizing its shutdown
+                            if (IsServer)
+                            {
+                                ProcessServerShutdown();
+                            }
+                            else
+                            {
+                                // Clients just disconnect immediately
+                                ShutdownInternal();
+                            }
                         }
                     }
                     break;
             }
         }
 
+        /// <summary>
+        /// Used to provide a graceful shutdown sequence
+        /// </summary>
+        internal enum ServerShutdownStates
+        {
+            None,
+            WaitForClientDisconnects,
+            InternalShutdown,
+            ShuttingDown,
+        };
+
+        internal ServerShutdownStates ServerShutdownState;
+        private float m_ShutdownTimeout;
+
+        /// <summary>
+        /// This is a "soft shutdown" where the host or server will disconnect
+        /// all clients, with a provided reasons, prior to invoking its final
+        /// internal shutdown.
+        /// </summary>
+        internal void ProcessServerShutdown()
+        {
+            var minClientCount = IsHost ? 2 : 1;
+            switch (ServerShutdownState)
+            {
+                case ServerShutdownStates.None:
+                    {
+                        if (ConnectedClients.Count >= minClientCount)
+                        {
+                            var hostServer = IsHost ? "host" : "server";
+                            var disconnectReason = $"Disconnected due to {hostServer} shutting down.";
+                            for (int i = ConnectedClientsIds.Count - 1; i >= 0; i--)
+                            {
+                                var clientId = ConnectedClientsIds[i];
+                                if (clientId == ServerClientId)
+                                {
+                                    continue;
+                                }
+                                ConnectionManager.DisconnectClient(clientId, disconnectReason);
+                            }
+                            ServerShutdownState = ServerShutdownStates.WaitForClientDisconnects;
+                            m_ShutdownTimeout = Time.realtimeSinceStartup + 5.0f;
+                        }
+                        else
+                        {
+                            ServerShutdownState = ServerShutdownStates.InternalShutdown;
+                            ProcessServerShutdown();
+                        }
+                        break;
+                    }
+                case ServerShutdownStates.WaitForClientDisconnects:
+                    {
+                        if (ConnectedClients.Count < minClientCount || m_ShutdownTimeout < Time.realtimeSinceStartup)
+                        {
+                            ServerShutdownState = ServerShutdownStates.InternalShutdown;
+                            ProcessServerShutdown();
+                        }
+                        break;
+                    }
+                case ServerShutdownStates.InternalShutdown:
+                    {
+                        ServerShutdownState = ServerShutdownStates.ShuttingDown;
+                        ShutdownInternal();
+                        break;
+                    }
+            }
+        }
+
         /// <summary>
         /// The client id used to represent the server
         /// </summary>
@@ -704,6 +780,12 @@ public int MaximumFragmentedMessageSize
 
         internal void Initialize(bool server)
         {
+            // Make sure the ServerShutdownState is reset when initializing
+            if (server)
+            {
+                ServerShutdownState = ServerShutdownStates.None;
+            }
+
             // Don't allow the user to start a network session if the NetworkManager is
             // still parented under another GameObject
             if (NetworkManagerCheckForParent(true))
@@ -970,7 +1052,6 @@ public bool StartHost()
         private void HostServerInitialize()
         {
             LocalClientId = ServerClientId;
-            //ConnectionManager.ConnectedClientIds.Add(ServerClientId);
             NetworkMetrics.SetConnectionId(LocalClientId);
             MessageManager.SetLocalClientId(LocalClientId);
 
@@ -1051,11 +1132,6 @@ public void Shutdown(bool discardMessageQueue = false)
                     MessageManager.StopProcessing = discardMessageQueue;
                 }
             }
-
-            if (NetworkConfig != null && NetworkConfig.NetworkTransport != null)
-            {
-                NetworkConfig.NetworkTransport.OnTransportEvent -= ConnectionManager.HandleNetworkEvent;
-            }
         }
 
         // Ensures that the NetworkManager is cleaned up before OnDestroy is run on NetworkObjects and NetworkBehaviours when unloading a scene with a NetworkManager

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs

@@ -100,7 +100,15 @@ protected void MarkNetworkBehaviourDirty()
                                  "Are you modifying a NetworkVariable before the NetworkObject is spawned?");
                 return;
             }
-
+            if (m_NetworkBehaviour.NetworkManager.ShutdownInProgress)
+            {
+                if (m_NetworkBehaviour.NetworkManager.LogLevel <= LogLevel.Developer)
+                {
+                    Debug.LogWarning($"NetworkVariable is written to during the NetworkManager shutdown! " +
+                 "Are you modifying a NetworkVariable within a NetworkBehaviour.OnDestroy or NetworkBehaviour.OnDespawn method?");
+                }
+                return;
+            }
             m_NetworkBehaviour.NetworkManager.BehaviourUpdater.AddForUpdate(m_NetworkBehaviour.NetworkObject);
         }