fix: shutdown and shutdown notification issues [MTT-7791] [MTT-7540]

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