fix: fixing connection management test [MTT-4492][MTT-6242] (#826)

* Adding OnServerShutdown event to ConnectionManager and ConnectionStates

* Fixing failing test

Author: LPLafontaineB

Assets/Scripts/ConnectionManagement/ConnectionManager.cs

@@ -98,6 +98,7 @@ void Start()
             NetworkManager.OnServerStarted += OnServerStarted;
             NetworkManager.ConnectionApprovalCallback += ApprovalCheck;
             NetworkManager.OnTransportFailure += OnTransportFailure;
+            NetworkManager.OnServerStopped += OnServerStopped;
         }
 
         void OnDestroy()
@@ -107,6 +108,7 @@ void OnDestroy()
             NetworkManager.OnServerStarted -= OnServerStarted;
             NetworkManager.ConnectionApprovalCallback -= ApprovalCheck;
             NetworkManager.OnTransportFailure -= OnTransportFailure;
+            NetworkManager.OnServerStopped -= OnServerStopped;
         }
 
         internal void ChangeState(ConnectionState nextState)
@@ -146,6 +148,11 @@ void OnTransportFailure()
             m_CurrentState.OnTransportFailure();
         }
 
+        void OnServerStopped(bool _) // we don't need this parameter as the ConnectionState already carries the relevant information
+        {
+            m_CurrentState.OnServerStopped();
+        }
+
         public void StartClientLobby(string playerName)
         {
             m_CurrentState.StartClientLobby(playerName);

Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs

@@ -36,10 +36,10 @@ public override void OnClientConnected(ulong _)
         public override void OnClientDisconnect(ulong _)
         {
             // client ID is for sure ours here
-            StartingClientFailedAsync();
+            StartingClientFailed();
         }
 
-        protected void StartingClientFailedAsync()
+        void StartingClientFailed()
         {
             var disconnectReason = m_ConnectionManager.NetworkManager.DisconnectReason;
             if (string.IsNullOrEmpty(disconnectReason))
@@ -72,7 +72,7 @@ internal async Task ConnectClientAsync()
             {
                 Debug.LogError("Error connecting client, see following exception");
                 Debug.LogException(e);
-                StartingClientFailedAsync();
+                StartingClientFailed();
                 throw;
             }
         }

Assets/Scripts/ConnectionManagement/ConnectionState/ClientReconnectingState.cs

@@ -122,8 +122,8 @@ IEnumerator ReconnectCoroutine()
             if (!reconnectingSetupTask.IsFaulted && reconnectingSetupTask.Result.success)
             {
                 // If this fails, the OnClientDisconnect callback will be invoked by Netcode
-                var connectingToRelay = ConnectClientAsync();
-                yield return new WaitUntil(() => connectingToRelay.IsCompleted);
+                var connectingTask = ConnectClientAsync();
+                yield return new WaitUntil(() => connectingTask.IsCompleted);
             }
             else
             {

Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs

@@ -39,5 +39,7 @@ public virtual void OnUserRequestedShutdown() { }
         public virtual void ApprovalCheck(NetworkManager.ConnectionApprovalRequest request, NetworkManager.ConnectionApprovalResponse response) { }
 
         public virtual void OnTransportFailure() { }
+
+        public virtual void OnServerStopped() { }
     }
 }

Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs

@@ -47,11 +47,7 @@ public override void OnClientConnected(ulong clientId)
 
         public override void OnClientDisconnect(ulong clientId)
         {
-            if (clientId == m_ConnectionManager.NetworkManager.LocalClientId)
-            {
-                m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline);
-            }
-            else
+            if (clientId != m_ConnectionManager.NetworkManager.LocalClientId)
             {
                 var playerId = SessionManager<SessionPlayerData>.Instance.GetPlayerId(clientId);
                 if (playerId != null)
@@ -80,6 +76,12 @@ public override void OnUserRequestedShutdown()
             m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline);
         }
 
+        public override void OnServerStopped()
+        {
+            m_ConnectStatusPublisher.Publish(ConnectStatus.GenericDisconnect);
+            m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline);
+        }
+
         /// <summary>
         /// This logic plugs into the "ConnectionApprovalResponse" exposed by Netcode.NetworkManager. It is run every time a client connects to us.
         /// The complementary logic that runs when the client starts its connection can be found in ClientConnectingState.

Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs

@@ -33,20 +33,6 @@ public override void Enter()
 
         public override void Exit() { }
 
-        public override void OnClientDisconnect(ulong clientId)
-        {
-            if (clientId == m_ConnectionManager.NetworkManager.LocalClientId)
-            {
-                StartHostFailed();
-            }
-        }
-
-        void StartHostFailed()
-        {
-            m_ConnectStatusPublisher.Publish(ConnectStatus.StartHostFailed);
-            m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline);
-        }
-
         public override void OnServerStarted()
         {
             m_ConnectStatusPublisher.Publish(ConnectStatus.Success);
@@ -72,6 +58,11 @@ public override void ApprovalCheck(NetworkManager.ConnectionApprovalRequest requ
             }
         }
 
+        public override void OnServerStopped()
+        {
+            StartHostFailed();
+        }
+
         async void StartHost()
         {
             try
@@ -82,7 +73,7 @@ async void StartHost()
                 // NGO's StartHost launches everything
                 if (!m_ConnectionManager.NetworkManager.StartHost())
                 {
-                    OnClientDisconnect(m_ConnectionManager.NetworkManager.LocalClientId);
+                    StartHostFailed();
                 }
             }
             catch (Exception)
@@ -91,5 +82,11 @@ async void StartHost()
                 throw;
             }
         }
+
+        void StartHostFailed()
+        {
+            m_ConnectStatusPublisher.Publish(ConnectStatus.StartHostFailed);
+            m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline);
+        }
     }
 }

Assets/Tests/Runtime/ConnectionManagementTests.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections;
+using System.Collections.Generic;
 using NUnit.Framework;
 using Unity.BossRoom.ConnectionManagement;
 using Unity.BossRoom.Infrastructure;
@@ -355,8 +356,7 @@ public IEnumerator UnexpectedClientDisconnect_ClientReconnectingSuccessfully()
             subscriptions.Dispose();
         }
 
-
-        [UnityTest, Ignore("Test fails because shutdowns do not invoke OnClientDisconnect on the host, so the ConnectionManager doesn't properly transition to the Offline state")]
+        [UnityTest]
         public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect()
         {
             StartHost();
@@ -368,6 +368,7 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect()
             AssertAllClientsAreConnected();
 
             var nbReconnectingMsgsReceived = 0;
+            var nbGenericDisconnectMsgReceived = 0;
             var subscriptions = new DisposableGroup();
 
             for (int i = 0; i < NumberOfClients; i++)
@@ -377,8 +378,18 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect()
                     // ignoring the first success message that is in the buffer
                     if (message != ConnectStatus.Success)
                     {
-                        Assert.AreEqual(ConnectStatus.Reconnecting, message, "Received unexpected ConnectStatus message.");
-                        nbReconnectingMsgsReceived++;
+                        var possibleMessages = new List<ConnectStatus>();
+                        possibleMessages.Add(ConnectStatus.Reconnecting);
+                        possibleMessages.Add(ConnectStatus.GenericDisconnect);
+                        Assert.Contains(message, possibleMessages, "Received unexpected ConnectStatus message.");
+                        if (message == ConnectStatus.Reconnecting)
+                        {
+                            nbReconnectingMsgsReceived++;
+                        }
+                        else if (message == ConnectStatus.GenericDisconnect)
+                        {
+                            nbGenericDisconnectMsgReceived++;
+                        }
                     }
                 }));
             }
@@ -396,22 +407,32 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect()
                 Assert.IsFalse(m_ClientNetworkManagers[clientId].IsConnectedClient, $"Client{clientId} has not shut down properly after losing connection.");
             }
 
-            // Waiting for clients to fail to automatically reconnect
+            var maxNbReconnectionAttempts = 0;
+
             for (var i = 0; i < NumberOfClients; i++)
             {
-                for (var j = 0; j < m_ClientConnectionManagers[i].NbReconnectAttempts; j++)
+                var nbReconnectionAttempts = m_ClientConnectionManagers[i].NbReconnectAttempts;
+                maxNbReconnectionAttempts = Math.Max(maxNbReconnectionAttempts, nbReconnectionAttempts);
+                for (var j = 0; j < nbReconnectionAttempts; j++)
                 {
                     // Expecting this error for each reconnection attempt for each client
                     LogAssert.Expect(LogType.Error, k_FailedToConnectToServerErrorMessage);
                 }
             }
-            yield return WaitForClientsConnectedOrTimeOut();
-            for (var i = 0; i < NumberOfClients; i++)
+
+            // Waiting for clients to fail to automatically reconnect. We wait once for each reconnection attempt.
+            for (var i = 0; i < maxNbReconnectionAttempts; i++)
             {
-                Assert.IsFalse(m_ClientNetworkManagers[i].IsConnectedClient, $"Client{i} is connected while no server is running.");
+                yield return WaitForClientsConnectedOrTimeOut();
+                for (var j = 0; j < NumberOfClients; j++)
+                {
+                    Assert.IsFalse(m_ClientNetworkManagers[j].IsConnectedClient, $"Client{j} is connected while no server is running.");
+                }
+
             }
 
             Assert.AreEqual(NumberOfClients, nbReconnectingMsgsReceived, "Not all clients received a Reconnecting message.");
+            Assert.AreEqual(NumberOfClients, nbGenericDisconnectMsgReceived, "Not all clients received a GenericDisconnect message.");
             subscriptions.Dispose();
         }
 

CHANGELOG.md

@@ -8,6 +8,9 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ## [unreleased] - yyyy-mm-dd
 
+### Added
+* Added OnServerStopped event to ConnectionManager and ConnectionState (#826). This allows for the detection of an unexpected shutdown on the server side.
+
 ### Changed
 * Replaced our polling for lobby updates with a subscription to the new Websocket based LobbyEvents (#805). This saves up a significant amount of bandwidth usage to and from the service, since updates are infrequent in this game. Now clients and hosts only use up bandwidth on the Lobby service when it is needed. With polling, we used to send a GET request per client once every 2s. The responses were between ~550 bytes and 900 bytes, so if we suppose an average of 725 bytes and 100 000 concurrent users (CCU), this amounted to around 725B * 30 calls per minute * 100 000 CCU = 2.175 GB per minute. Scaling this to a month would get us 93.96 TB per month. In our case, since the only changes to the lobbies happen when a user connects or disconnects, most of that data was not necessary and can be saved to reduce bandwidth usage. Since the cost of using the Lobby service depends on bandwidth usage, this would also save money on an actual game.
 * Simplified reconnection flow by offloading responsibility to ConnectionMethod (#804). Now the ClientReconnectingState uses the ConnectionMethod it is configured with to handle setting up reconnection (i.e. reconnecting to the Lobby before trying to reconnect to the Relay server if it is using Relay and Lobby). It can now also fail early and stop retrying if the lobby doesn't exist anymore.
@@ -27,6 +30,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 * ClientConnectedState now inherits from OnlineState instead of the base ConnectionState (#801)
 * UpdateRunner now sends the right value for deltaTime when updating its subscribers (#805)
 * Inputs are better sanitized when entering IP address and port (#821). Now all invalid characters are prevented, and UnityTransport's NetworkEndpoint.TryParse is used to verify the validity of the IP address and port that are entered before making the join/host button interactable.
+* Fixed failing connection management test (#826). This test had to be ignored previously because there was no mechanism to detect unexpected server shutdowns. With the OnServerStopped callback introduced in NGO 1.4.0, this is no longer an issue. 
 * Decoupled SceneLoaderWrapper and ConnectionStates (#830). The OnServerStarted and OnClientStarted callbacks available in NGO 1.4.0 allows us to remove the need for an external method to initialize the SceneLoaderWrapper after starting a NetworkingSession.
 
 ## [2.0.4] - 2022-12-13