fix: client and server disconnect warnings

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,9 +14,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Added a flag to override command-line arguments (port and ip) in `SetConnectionData`. (#3760)
 - Added a command-line singleton to parse environment command-line arguments. (#3760)
 
-
 ### Changed
 
+- Changed when a server is disconnecting a client with a reason it now defers the complete transport disconnect sequence until the end of the frame after the server's transport has sent all pending outbound messages. (#3786)
 
 ### Deprecated
 
@@ -27,6 +27,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Fixed
 
 - Fixed issues with the "Client-server quickstart for Netcode for GameObjects" script having static methods and properties. (#3787)
+- Fixed issue where a warning message was being logged upon a client disconnecting from a server when the log level is set to developer. (#3786)
+- Fixed issue where the server or host would no longer have access to the transport id to client id table when processing a transport level client disconnect event. (#3786)
 - Fixed issue where invoking an RPC, on another `NetworkBehaviour` associated with the same `NetworkObject` that is ordered before the `NetworkBehaviour` invoking the RPC, during `OnNetworkSpawn` could throw an exception if scene management is disabled. (#3782)
 - Fixed issue where the `Axis to Synchronize` toggles didn't work with multi object editing in `NetworkTransform`. (#3781)
 - Fixed issue where using the dedicated server package would override all attempts to change the port by code. (#3760)

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

@@ -362,11 +362,6 @@ internal void RemovePendingClient(ulong clientId)
                 return (clientId, true);
             }
 
-            if (NetworkLog.CurrentLogLevel == LogLevel.Developer)
-            {
-                NetworkLog.LogWarning($"Trying to get the NGO client ID map for the transport ID ({transportId}) but did not find the map entry! Returning default transport ID value.");
-            }
-
             return (default, false);
         }
 
@@ -488,6 +483,15 @@ internal void HandleNetworkEvent(NetworkEvent networkEvent, ulong transportClien
             }
         }
 
+        /// <summary>
+        /// Client's save their assigned transport id.
+        /// </summary>
+        /// <remarks>
+        /// Added to be able to appropriately log the client's transport
+        /// id when it is shutdown or disconnected.
+        /// </remarks>
+        private ulong m_LocalClientTransportId;
+
         /// <summary>
         /// Handles a <see cref="NetworkEvent.Connect"/> event.
         /// </summary>
@@ -508,6 +512,8 @@ internal void ConnectEventHandler(ulong transportClientId)
             }
             else
             {
+                // Cache the local client's transport id.
+                m_LocalClientTransportId = transportClientId;
                 clientId = NetworkManager.ServerClientId;
             }
 
@@ -585,9 +591,14 @@ private void GenerateDisconnectInformation(ulong clientId, ulong transportClient
         /// </summary>
         internal void DisconnectEventHandler(ulong transportClientId)
         {
-            var (clientId, wasConnectedClient) = TransportIdCleanUp(transportClientId);
-            if (!wasConnectedClient)
+            // Check to see if the client has already been removed from the table but
+            // do not remove it just yet.
+            var (clientId, isConnectedClient) = TransportIdToClientId(transportClientId);
+
+            // If the client is not registered and we are the server
+            if (!isConnectedClient && NetworkManager.IsServer)
             {
+                // Then exit early
                 return;
             }
 
@@ -622,17 +633,12 @@ internal void DisconnectEventHandler(ulong transportClientId)
             {
                 // We need to process the disconnection before notifying
                 OnClientDisconnectFromServer(clientId);
-
-                // Now notify the client has disconnected
-                InvokeOnClientDisconnectCallback(clientId);
-
-                if (LocalClient.IsHost)
-                {
-                    InvokeOnPeerDisconnectedCallback(clientId);
-                }
             }
             else
             {
+                // Client's clean up their transport id separately from the server.
+                TransportIdCleanUp(transportClientId);
+
                 // Notify local client of disconnection
                 InvokeOnClientDisconnectCallback(clientId);
 
@@ -793,12 +799,15 @@ private IEnumerator ApprovalTimeout(ulong clientId)
         /// </summary>
         internal void ApproveConnection(ref ConnectionRequestMessage connectionRequestMessage, ref NetworkContext context)
         {
+            if (ConnectionApprovalCallback == null)
+            {
+                return;
+            }
             // Note: Delegate creation allocates.
             // Note: ToArray() also allocates. :(
             var response = new NetworkManager.ConnectionApprovalResponse();
             ClientsToApprove[context.SenderId] = response;
-
-            ConnectionApprovalCallback(
+            ConnectionApprovalCallback?.Invoke(
                 new NetworkManager.ConnectionApprovalRequest
                 {
                     Payload = connectionRequestMessage.ConnectionData,
@@ -852,13 +861,6 @@ internal void ProcessPendingApprovals()
             }
         }
 
-        /// <summary>
-        /// Adding this because message hooks cannot happen fast enough under certain scenarios
-        /// where the message is sent and responded to before the hook is in place.
-        /// </summary>
-        internal bool MockSkippingApproval;
-
-
         /// <summary>
         /// Server Side: Handles the denial of a client who sent a connection request
         /// </summary>
@@ -873,12 +875,36 @@ private void HandleConnectionDisconnect(ulong ownerClientId, string reason = "")
                 {
                     Reason = reason
                 };
-                SendMessage(ref disconnectReason, NetworkDelivery.Reliable, ownerClientId);
+                SendMessage(ref disconnectReason, MessageDeliveryType<DisconnectReasonMessage>.DefaultDelivery, ownerClientId);
+                m_ClientsToDisconnect.Add(ownerClientId);
+                return;
             }
 
             DisconnectRemoteClient(ownerClientId);
         }
 
+        private List<ulong> m_ClientsToDisconnect = new List<ulong>();
+
+        internal void ProcessClientsToDisconnect()
+        {
+            if (m_ClientsToDisconnect.Count == 0)
+            {
+                return;
+            }
+            foreach (var clientId in m_ClientsToDisconnect)
+            {
+                try
+                {
+                    DisconnectRemoteClient(clientId);
+                }
+                catch (Exception ex)
+                {
+                    Debug.LogException(ex);
+                }
+            }
+            m_ClientsToDisconnect.Clear();
+        }
+
         /// <summary>
         /// Server Side: Handles the approval of a client
         /// </summary>
@@ -939,12 +965,6 @@ internal void HandleConnectionApproval(ulong ownerClientId, bool createPlayerObj
             // Server doesn't send itself the connection approved message
             if (ownerClientId != NetworkManager.ServerClientId)
             {
-                if (MockSkippingApproval)
-                {
-                    NetworkLog.LogInfo("Mocking server not responding with connection approved...");
-                    return;
-                }
-
                 SendConnectionApprovedMessage(ownerClientId);
 
                 // If scene management is disabled, then we are done and notify the local host-server the client is connected
@@ -1040,7 +1060,7 @@ private void SendConnectionApprovedMessage(ulong approvedClientId)
                 }
             }
 
-            SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, approvedClientId);
+            SendMessage(ref message, MessageDeliveryType<ConnectionApprovedMessage>.DefaultDelivery, approvedClientId);
 
             message.MessageVersions.Dispose();
             message.ConnectedClientIds.Dispose();
@@ -1111,13 +1131,9 @@ internal NetworkClient AddClient(ulong clientId)
                 return ConnectedClients[clientId];
             }
 
-            var networkClient = LocalClient;
-
             // If this is not the local client then create a new one
-            if (clientId != NetworkManager.LocalClientId)
-            {
-                networkClient = new NetworkClient();
-            }
+            var networkClient = clientId == NetworkManager.LocalClientId ? LocalClient : new NetworkClient();
+
             networkClient.SetRole(clientId == NetworkManager.ServerClientId, isClient: true, NetworkManager);
             networkClient.ClientId = clientId;
             if (!ConnectedClients.ContainsKey(clientId))
@@ -1224,6 +1240,14 @@ internal void OnClientDisconnectFromServer(ulong clientId)
             // clean up as everything that needs to be destroyed will be during shutdown.
             if (NetworkManager.ShutdownInProgress && clientId == NetworkManager.ServerClientId)
             {
+                // Now notify the client has disconnected.
+                // (transport id cleanup is handled within)
+                InvokeOnClientDisconnectCallback(clientId);
+
+                if (LocalClient.IsHost)
+                {
+                    InvokeOnPeerDisconnectedCallback(clientId);
+                }
                 return;
             }
 
@@ -1392,8 +1416,20 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                 }
 
                 ConnectedClientIds.Remove(clientId);
-                var message = new ClientDisconnectedMessage { ClientId = clientId };
-                MessageManager?.SendMessage(ref message, MessageDeliveryType<ClientDisconnectedMessage>.DefaultDelivery, ConnectedClientIds);
+
+                if (MessageManager != null)
+                {
+                    var message = new ClientDisconnectedMessage { ClientId = clientId };
+                    foreach (var sendToId in ConnectedClientIds)
+                    {
+                        // Do not send a disconnect message to ourself
+                        if (sendToId == NetworkManager.LocalClientId)
+                        {
+                            continue;
+                        }
+                        MessageManager.SendMessage(ref message, MessageDeliveryType<ClientDisconnectedMessage>.DefaultDelivery, sendToId);
+                    }
+                }
 
                 // Used for testing/validation purposes only
                 // Promote a new session owner when the ENABLE_DAHOST_AUTOPROMOTE_SESSION_OWNER scripting define is set
@@ -1404,17 +1440,18 @@ internal void OnClientDisconnectFromServer(ulong clientId)
             var (transportId, idExists) = ClientIdToTransportId(clientId);
             if (idExists)
             {
-                NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId);
-
-                InvokeOnClientDisconnectCallback(clientId);
-
-                if (LocalClient.IsHost)
+                // Clean up the transport to client (and vice versa) mappings
+                var (transportIdDisconnected, wasRemoved) = TransportIdCleanUp(transportId);
+                if (wasRemoved)
                 {
-                    InvokeOnPeerDisconnectedCallback(clientId);
-                }
+                    NetworkManager.NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId);
+                    InvokeOnClientDisconnectCallback(clientId);
 
-                // Clean up the transport to client (and vice versa) mappings
-                TransportIdCleanUp(transportId);
+                    if (LocalClient.IsHost)
+                    {
+                        InvokeOnPeerDisconnectedCallback(clientId);
+                    }
+                }
             }
 
             // Assure the client id is no longer in the pending clients list
@@ -1462,16 +1499,6 @@ internal void DisconnectClient(ulong clientId, string reason = null)
                 return;
             }
 
-            if (!string.IsNullOrEmpty(reason))
-            {
-                var disconnectReason = new DisconnectReasonMessage
-                {
-                    Reason = reason
-                };
-                SendMessage(ref disconnectReason, NetworkDelivery.Reliable, clientId);
-            }
-
-            Transport.ClosingRemoteConnection();
             var transportId = ClientIdToTransportId(clientId);
             if (transportId.Item2)
             {
@@ -1491,6 +1518,7 @@ internal void DisconnectClient(ulong clientId, string reason = null)
         internal void Initialize(NetworkManager networkManager)
         {
             // Prepare for a new session
+            m_LocalClientTransportId = 0;
             LocalClient.IsApproved = false;
             m_PendingClients.Clear();
             ConnectedClients.Clear();
@@ -1524,8 +1552,9 @@ internal void Shutdown()
             {
                 Transport.ShuttingDown();
                 var clientId = NetworkManager ? NetworkManager.LocalClientId : NetworkManager.ServerClientId;
-                var transportId = ClientIdToTransportId(clientId);
-                GenerateDisconnectInformation(clientId, transportId.Item1, $"{nameof(NetworkConnectionManager)} was shutdown.");
+                // Server and host just log 0 for their transport id while clients will log their cached m_LocalClientTransportId
+                var transportId = clientId == NetworkManager.ServerClientId ? 0 : m_LocalClientTransportId;
+                GenerateDisconnectInformation(clientId, transportId, $"{nameof(NetworkConnectionManager)} was shutdown.");
             }
 
             if (LocalClient.IsServer)

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

@@ -346,7 +346,6 @@ public void NetworkUpdate(NetworkUpdateStage updateStage)
                         AnticipationSystem.SetupForUpdate();
                         MessageManager.ProcessIncomingMessageQueue();
 
-                        MessageManager.CleanupDisconnectedClients();
                         AnticipationSystem.ProcessReanticipation();
 #if COM_UNITY_MODULES_PHYSICS || COM_UNITY_MODULES_PHYSICS2D
                         foreach (var networkObjectEntry in NetworkTransformFixedUpdate)
@@ -478,6 +477,18 @@ public void NetworkUpdate(NetworkUpdateStage updateStage)
                         // This is "ok" to invoke when not processing messages since it is just cleaning up messages that never got handled within their timeout period.
                         DeferredMessageManager.CleanupStaleTriggers();
 
+                        if (IsServer)
+                        {
+                            // Process any pending clients that need to be disconnected.
+                            // This is typically a disconnect with reason scenario where
+                            // we want the disconnect reason message to be sent prior to
+                            // completely shutting down the endpoint.
+                            ConnectionManager.ProcessClientsToDisconnect();
+                        }
+
+                        // Clean up disconnected clients last
+                        MessageManager.CleanupDisconnectedClients();
+
                         if (m_ShuttingDown)
                         {
                             // Host-server will disconnect any connected clients prior to finalizing its shutdown

com.unity.netcode.gameobjects/Runtime/Messaging/NetworkMessageManager.cs

@@ -492,6 +492,11 @@ private void CleanupDisconnectedClient(ulong clientId)
 
         internal void CleanupDisconnectedClients()
         {
+            if (m_DisconnectedClients.Count == 0)
+            {
+                return;
+            }
+
             foreach (var clientId in m_DisconnectedClients)
             {
                 CleanupDisconnectedClient(clientId);

com.unity.netcode.gameobjects/Tests/Runtime/ClientOnlyConnectionTests.cs

@@ -32,9 +32,9 @@ public void Setup()
             // Default is 1000ms per connection attempt and 60 connection attempts (60s)
             // Currently there is no easy way to set these values other than in-editor
             var unityTransport = m_NetworkManagerGameObject.AddComponent<UnityTransport>();
-            unityTransport.ConnectTimeoutMS = 1000;
+            unityTransport.ConnectTimeoutMS = 500;
             unityTransport.MaxConnectAttempts = 1;
-            m_TimeoutHelper = new TimeoutHelper(2);
+            m_TimeoutHelper = new TimeoutHelper(4);
             m_ClientNetworkManager.NetworkConfig.NetworkTransport = unityTransport;
         }
 
@@ -49,7 +49,6 @@ public IEnumerator ClientFailsToConnect()
 
             // Unity Transport throws an error when it times out
             LogAssert.Expect(LogType.Error, "Failed to connect to server.");
-
             yield return NetcodeIntegrationTest.WaitForConditionOrTimeOut(() => m_WasDisconnected, m_TimeoutHelper);
             Assert.False(m_TimeoutHelper.TimedOut, "Timed out waiting for client to timeout waiting to connect!");