fix: client and server disconnect warnings (#3786)

* fix

This fix prevents the warning message from being logged on the client side when shutting down from the client side and the developer log level is set.
This fix also assures that a server or host does not send itself a disconnect message.
This fix separated the transport id clean up between client and host/server upon handling a disconnect event which assures the server/host will have access to this transport id when processing a client that disconnected.

* update

Adding change log entries.

* test

Adding test to check that no warning messages are logged upon a client disconnecting itself from a session.

* refactor and fix

Fixing several issues with the disconnect flow.
Not removing the transport id until later. The server now only does this within OnClientDisconnectFromServer.
When disconnecting a client with a reason, the client is not completely disconnected until the end of the frame when the server has finished sending all pending messages.
Fixing some missed MessageDeliveryType replacements.
Removing the MockSkippingApproval as it is no longer needed with the fixes in place.

* test

Did a bunch of cleanup on several connection tests to improve stability and use more recent integration test features.
ClientOnlyConnectionTests reduced the transport connection timeout and increased the timeout helper.
ConnectionApprovalTimeoutTests needed to have the server and client timeout values vary depending upon the test type.
PeerDisconnectCallbackTests needed to trap for the peer disconnecting.

* style

Removing whitespaces

* update

adding one more change log entry

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -19,9 +19,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Changed
 
 - First pass of CoreCLR engine API changes. (#3799)
-- Changed NetworkAnimator to use the `RpcAttribute` along with the appropriate `SendTo` parameter. (#3586)
+- 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)
 - Improve performance of `NetworkTransformState`. (#3770)
-
+- Changed NetworkAnimator to use the `RpcAttribute` along with the appropriate `SendTo` parameter. (#3586)
 
 ### Deprecated
 
@@ -32,6 +32,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!");