fix: CMB Service not disconnecting

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -77,6 +77,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- CMB service no longer waits for a timeout before disconnecting on an invalid ConnectionRequest. (#3812)
 - Initialization errors with NetworkAnimator. (#3767)
 - Multiple disconnect events from the same transport will no longer disconnect the host. (#3707)
 - Fixed NetworkTransform state synchronization issue when `NetworkTransform.SwitchTransformSpaceWhenParented` is enabled and the associated NetworkObject is parented multiple times in a single frame or within a couple of frames. (#3664)

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

@@ -492,6 +492,8 @@ internal void HandleNetworkEvent(NetworkEvent networkEvent, ulong transportClien
         /// </remarks>
         private ulong m_LocalClientTransportId;
 
+        internal ulong LocalClientTransportId => m_LocalClientTransportId;
+
         /// <summary>
         /// Handles a <see cref="NetworkEvent.Connect"/> event.
         /// </summary>
@@ -596,7 +598,7 @@ internal void DisconnectEventHandler(ulong transportClientId)
             var (clientId, isConnectedClient) = TransportIdToClientId(transportClientId);
 
             // If the client is not registered and we are the server
-            if (!isConnectedClient && NetworkManager.IsServer)
+            if (!isConnectedClient)
             {
                 // Then exit early
                 return;

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/DisconnectReasonMessage.cs

@@ -1,3 +1,6 @@
+using System.Collections;
+using UnityEngine;
+
 namespace Unity.Netcode
 {
     internal struct DisconnectReasonMessage : INetworkMessage
@@ -37,10 +40,24 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context, int
 
         public void Handle(ref NetworkContext context)
         {
+            var networkManager = (NetworkManager)context.SystemOwner;
             // Always apply the server-side generated disconnect reason to the server specific disconnect reason.
             // This is combined with the additional disconnect information when getting NetworkManager.DisconnectReason
             // (NetworkConnectionManager.DisconnectReason).
-            ((NetworkManager)context.SystemOwner).ConnectionManager.ServerDisconnectReason = Reason;
+            networkManager.ConnectionManager.ServerDisconnectReason = Reason;
+
+            if (networkManager.NetworkConfig.UseCMBService)
+            {
+                networkManager.StartCoroutine(HandleDisconnectAfterReason(networkManager));
+            }
+        }
+
+        private IEnumerator HandleDisconnectAfterReason(NetworkManager networkManager)
+        {
+            yield return new WaitForFixedUpdate();
+
+            var connectionManager = networkManager.ConnectionManager;
+            connectionManager.DisconnectEventHandler(connectionManager.LocalClientTransportId);
         }
     };
 }

com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/SessionVersionConnectionRequest.cs

@@ -1,6 +1,7 @@
 using System.Collections;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine;
 using UnityEngine.TestTools;
 
 namespace Unity.Netcode.RuntimeTests
@@ -9,52 +10,36 @@ internal class SessionVersionConnectionRequest : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 0;
 
-        // TODO: [CmbServiceTests] Adapt to run with the service
-        protected override bool UseCMBService()
-        {
-            return false;
-        }
+        // Use a specific version for the CMB tests
+        // The CMB service has more detailed versioning logic. Not all lower versions are invalid to connect with the higher version.
+        // This version will not connect with the version lower.
+        private const int k_ValidCMBVersion = 5;
 
         public SessionVersionConnectionRequest() : base(NetworkTopologyTypes.DistributedAuthority, HostOrServer.DAHost) { }
 
         private bool m_UseValidSessionVersion;
         private bool m_ClientWasDisconnected;
-        private NetworkManager m_ClientNetworkManager;
+        private bool m_CanStartClients;
+
+        // Don't start automatically when using the CMB Service
+        // We want to customize the SessionVersion of the session owner before they connect
+        protected override bool CanStartServerAndClients() => !m_UseCmbService || m_CanStartClients;
 
         /// <summary>
         /// Callback used to mock the scenario where a client has an invalid session version
         /// </summary>
-        /// <returns><see cref="SessionConfig"/></returns>
-        private SessionConfig GetInavlidSessionConfig()
+        private SessionConfig GetInvalidSessionConfig()
         {
             var authority = GetAuthorityNetworkManager();
             return new SessionConfig(authority.SessionConfig.SessionVersion - 1);
         }
 
-        /// <summary>
-        /// Overriding this method allows us to configure the newly instantiated client's
-        /// NetworkManager prior to it being started.
-        /// </summary>
-        /// <param name="networkManager">the newly instantiated NetworkManager</param>
-        protected override void OnNewClientCreated(NetworkManager networkManager)
-        {
-            m_ClientWasDisconnected = false;
-            m_ClientNetworkManager = networkManager;
-            m_ClientNetworkManager.OnClientDisconnectCallback += OnClientDisconnectCallback;
-            if (!m_UseValidSessionVersion)
-            {
-                networkManager.OnGetSessionConfig = GetInavlidSessionConfig;
-            }
-            base.OnNewClientCreated(networkManager);
-        }
-
         /// <summary>
         /// Tracks if the client was disconnected or not
         /// </summary>
         private void OnClientDisconnectCallback(ulong clientId)
         {
             m_ClientWasDisconnected = true;
-            m_ClientNetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
         }
 
         /// <summary>
@@ -69,51 +54,76 @@ protected override bool ShouldWaitForNewClientToConnect(NetworkManager networkMa
         {
             return m_UseValidSessionVersion;
         }
-
-        internal enum SessionVersionType
-        {
-            Valid,
-            Invalid,
-        }
-
         /// <summary>
         /// Validates that when the client's session config version is valid a client will be
         /// allowed to connect and when it is not valid the client will be disconnected.
         /// </summary>
-        /// <remarks>
-        /// This is just a mock of the service logic to validate everything on the NGO side is
-        /// working correctly.
-        /// </remarks>
-        /// <param name="useValidSessionVersion">true = use valid session version | false = use invalid session version</param>
         [UnityTest]
-        public IEnumerator ValidateSessionVersion([Values] SessionVersionType type)
+        public IEnumerator ValidateSessionVersion()
         {
-            // Test client being disconnected due to invalid session version
-            m_UseValidSessionVersion = type == SessionVersionType.Valid;
-            yield return CreateAndStartNewClient();
-            yield return s_DefaultWaitForTick;
-            if (!m_UseValidSessionVersion)
+            if (m_UseCmbService)
             {
-                yield return WaitForConditionOrTimeOut(() => m_ClientWasDisconnected);
-                AssertOnTimeout("Client was not disconnected when it should have been!");
-                Assert.True(m_ClientNetworkManager.DisconnectReason.Contains(ConnectionRequestMessage.InvalidSessionVersionMessage), "Client did not receive the correct invalid session version message!");
+                var authority = GetAuthorityNetworkManager();
+                authority.OnGetSessionConfig = () => new SessionConfig(k_ValidCMBVersion);
+                m_CanStartClients = true;
+                yield return StartServerAndClients();
             }
-            else
+
+            /*
+             * Test client being disconnected due to invalid session version
+             */
+            m_UseValidSessionVersion = false;
+
+            // Create and setup client to use invalid session config
+            var invalidClient = CreateNewClient();
+            invalidClient.OnClientDisconnectCallback += OnClientDisconnectCallback;
+            invalidClient.OnGetSessionConfig = GetInvalidSessionConfig;
+
+            // Start client and wait for disconnect callback
+            m_ClientWasDisconnected = false;
+            yield return StartClient(invalidClient);
+            Assert.True(invalidClient.IsListening);
+            yield return s_DefaultWaitForTick;
+
+            var timeoutHelper = new TimeoutHelper(30f);
+            yield return WaitForConditionOrTimeOut(() => !invalidClient.IsListening, timeoutHelper);
+            AssertOnTimeout("Client is still listening when it should have been disconnected!", timeoutHelper);
+
+            yield return WaitForConditionOrTimeOut(() => m_ClientWasDisconnected);
+            AssertOnTimeout("Client was not disconnected when it should have been!");
+
+            var expectedReason = m_UseCmbService ? "incompatible ngo c# package versions for feature" : ConnectionRequestMessage.InvalidSessionVersionMessage;
+            Assert.That(invalidClient.DisconnectReason, Does.Contain(expectedReason), $"Client did not receive the correct invalid session version message! Received: {invalidClient.DisconnectReason}");
+
+            // Clean up invalid client
+            invalidClient.OnClientDisconnectCallback -= OnClientDisconnectCallback;
+            yield return StopOneClient(invalidClient, true);
+
+            /*
+             * Test a later client with a valid version
+             * They should connect as normal
+             */
+            m_UseValidSessionVersion = true;
+
+            // Create and setup client to use invalid session config
+            var lateJoin = CreateNewClient();
+            lateJoin.OnClientDisconnectCallback += OnClientDisconnectCallback;
+            if (m_UseCmbService)
             {
-                Assert.False(m_ClientWasDisconnected, "Client was disconnected when it was expected to connect!");
-                Assert.True(m_ClientNetworkManager.IsConnectedClient, "Client did not connect properly using the correct session version!");
+                lateJoin.OnGetSessionConfig = () => new SessionConfig(k_ValidCMBVersion);
             }
-        }
 
-        /// <summary>
-        /// Invoked at the end of each integration test pass.
-        /// Primarily used to clean up for the next pass.
-        /// </summary>
-        protected override IEnumerator OnTearDown()
-        {
-            m_ClientNetworkManager.OnClientDisconnectCallback -= OnClientDisconnectCallback;
-            m_ClientNetworkManager = null;
-            yield return base.OnTearDown();
+            // Start client and wait for disconnect callback
+            m_ClientWasDisconnected = false;
+            yield return StartClient(lateJoin);
+            yield return s_DefaultWaitForTick;
+
+            Assert.False(m_ClientWasDisconnected, "Client was disconnected when it was expected to connect!");
+            Assert.True(lateJoin.IsConnectedClient, "Client did not connect properly using the correct session version!");
+            Assert.That(GetAuthorityNetworkManager().ConnectedClientsIds, Has.Member(lateJoin.LocalClientId), "Newly joined client should be in connected list!");
+
+            // Clean up
+            lateJoin.OnClientDisconnectCallback -= OnClientDisconnectCallback;
         }
     }
 }