From 60503eeb2f77778761c6898d229cfe5e31e96264 Mon Sep 17 00:00:00 2001 From: LPLafontaineB Date: Mon, 24 Apr 2023 10:32:38 -0400 Subject: [PATCH 1/6] Adding OnServerShutdown event to ConnectionManager and ConnectionStates --- .../ConnectionManagement/ConnectionManager.cs | 7 +++++ .../ConnectionState/ConnectionState.cs | 2 ++ .../ConnectionState/HostingState.cs | 12 +++++---- .../ConnectionState/StartingHostState.cs | 27 +++++++++---------- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/Assets/Scripts/ConnectionManagement/ConnectionManager.cs b/Assets/Scripts/ConnectionManagement/ConnectionManager.cs index 3932807c8..a61f21740 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionManager.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionManager.cs @@ -98,6 +98,7 @@ void Start() NetworkManager.OnServerStarted += OnServerStarted; NetworkManager.ConnectionApprovalCallback += ApprovalCheck; NetworkManager.OnTransportFailure += OnTransportFailure; + NetworkManager.OnServerStopped += OnServerShutdown; } void OnDestroy() @@ -107,6 +108,7 @@ void OnDestroy() NetworkManager.OnServerStarted -= OnServerStarted; NetworkManager.ConnectionApprovalCallback -= ApprovalCheck; NetworkManager.OnTransportFailure -= OnTransportFailure; + NetworkManager.OnServerStopped -= OnServerShutdown; } internal void ChangeState(ConnectionState nextState) @@ -146,6 +148,11 @@ void OnTransportFailure() m_CurrentState.OnTransportFailure(); } + void OnServerShutdown(bool _) // we don't need this parameter as the ConnectionState already carries the relevant information + { + m_CurrentState.OnServerShutdown(); + } + public void StartClientLobby(string playerName) { m_CurrentState.StartClientLobby(playerName); diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs index 44251e45a..538956c06 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs +++ b/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 OnServerShutdown() { } } } diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs index daf1bcd58..f2f57cb45 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs @@ -50,11 +50,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.Instance.GetPlayerId(clientId); if (playerId != null) @@ -83,6 +79,12 @@ public override void OnUserRequestedShutdown() m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline); } + public override void OnServerShutdown() + { + m_ConnectStatusPublisher.Publish(ConnectStatus.GenericDisconnect); + m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline); + } + /// /// 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. diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs index eca47b3b6..73cb09499 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs +++ b/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 OnServerShutdown() + { + 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); + } } } From b9df9d4f271428f0b95d5c6a124ede7300b615f3 Mon Sep 17 00:00:00 2001 From: LPLafontaineB Date: Mon, 24 Apr 2023 10:33:44 -0400 Subject: [PATCH 2/6] renaming badly named method and variable --- .../ConnectionState/ClientConnectingState.cs | 6 +++--- .../ConnectionState/ClientReconnectingState.cs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs index be9a0512c..b3baada08 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionState/ClientConnectingState.cs @@ -37,10 +37,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)) @@ -75,7 +75,7 @@ internal async Task ConnectClientAsync() { Debug.LogError("Error connecting client, see following exception"); Debug.LogException(e); - StartingClientFailedAsync(); + StartingClientFailed(); throw; } } diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/ClientReconnectingState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/ClientReconnectingState.cs index 7da62da1b..3b7d0397e 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/ClientReconnectingState.cs +++ b/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 { From c57ad4f1a42708083a7ba80b64ec630b7b8ca117 Mon Sep 17 00:00:00 2001 From: LPLafontaineB Date: Mon, 24 Apr 2023 10:34:30 -0400 Subject: [PATCH 3/6] Fixing failing test --- .../Runtime/ConnectionManagementTests.cs | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/Assets/Tests/Runtime/ConnectionManagementTests.cs b/Assets/Tests/Runtime/ConnectionManagementTests.cs index 5254b95f7..008efbabf 100644 --- a/Assets/Tests/Runtime/ConnectionManagementTests.cs +++ b/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; @@ -357,8 +358,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(); @@ -370,6 +370,7 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect() AssertAllClientsAreConnected(); var nbReconnectingMsgsReceived = 0; + var nbGenericDisconnectMsgReceived = 0; var subscriptions = new DisposableGroup(); for (int i = 0; i < NumberOfClients; i++) @@ -379,8 +380,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(); + 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++; + } } })); } @@ -398,22 +409,32 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect() Assert.IsFalse(m_ClientNetworkManagers[clientId].IsConnectedClient, $"Client{clientId} has not shut down properly after losing connection."); } + var maxNbReconnectionAttempts = 0; + // Waiting for clients to fail to automatically reconnect 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++) + + 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(); } From 0c7e69b70039c2a10adc5d45473ae77d4e9b9e8e Mon Sep 17 00:00:00 2001 From: LPLafontaineB Date: Mon, 24 Apr 2023 10:39:27 -0400 Subject: [PATCH 4/6] renaming methods for consistency --- Assets/Scripts/ConnectionManagement/ConnectionManager.cs | 8 ++++---- .../ConnectionState/ConnectionState.cs | 2 +- .../ConnectionManagement/ConnectionState/HostingState.cs | 2 +- .../ConnectionState/StartingHostState.cs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Assets/Scripts/ConnectionManagement/ConnectionManager.cs b/Assets/Scripts/ConnectionManagement/ConnectionManager.cs index a61f21740..22068b375 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionManager.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionManager.cs @@ -98,7 +98,7 @@ void Start() NetworkManager.OnServerStarted += OnServerStarted; NetworkManager.ConnectionApprovalCallback += ApprovalCheck; NetworkManager.OnTransportFailure += OnTransportFailure; - NetworkManager.OnServerStopped += OnServerShutdown; + NetworkManager.OnServerStopped += OnServerStopped; } void OnDestroy() @@ -108,7 +108,7 @@ void OnDestroy() NetworkManager.OnServerStarted -= OnServerStarted; NetworkManager.ConnectionApprovalCallback -= ApprovalCheck; NetworkManager.OnTransportFailure -= OnTransportFailure; - NetworkManager.OnServerStopped -= OnServerShutdown; + NetworkManager.OnServerStopped -= OnServerStopped; } internal void ChangeState(ConnectionState nextState) @@ -148,9 +148,9 @@ void OnTransportFailure() m_CurrentState.OnTransportFailure(); } - void OnServerShutdown(bool _) // we don't need this parameter as the ConnectionState already carries the relevant information + void OnServerStopped(bool _) // we don't need this parameter as the ConnectionState already carries the relevant information { - m_CurrentState.OnServerShutdown(); + m_CurrentState.OnServerStopped(); } public void StartClientLobby(string playerName) diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs index 538956c06..eed9a87da 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionState/ConnectionState.cs @@ -40,6 +40,6 @@ public virtual void ApprovalCheck(NetworkManager.ConnectionApprovalRequest reque public virtual void OnTransportFailure() { } - public virtual void OnServerShutdown() { } + public virtual void OnServerStopped() { } } } diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs index f2f57cb45..dc6866ef0 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionState/HostingState.cs @@ -79,7 +79,7 @@ public override void OnUserRequestedShutdown() m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline); } - public override void OnServerShutdown() + public override void OnServerStopped() { m_ConnectStatusPublisher.Publish(ConnectStatus.GenericDisconnect); m_ConnectionManager.ChangeState(m_ConnectionManager.m_Offline); diff --git a/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs b/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs index 73cb09499..a752cda77 100644 --- a/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs +++ b/Assets/Scripts/ConnectionManagement/ConnectionState/StartingHostState.cs @@ -58,7 +58,7 @@ public override void ApprovalCheck(NetworkManager.ConnectionApprovalRequest requ } } - public override void OnServerShutdown() + public override void OnServerStopped() { StartHostFailed(); } From b768f271e89719671580e5d321f43477961f2e8e Mon Sep 17 00:00:00 2001 From: LPLafontaineB Date: Mon, 24 Apr 2023 10:47:07 -0400 Subject: [PATCH 5/6] Adding changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a71278746..5e9f051c6 100644 --- a/CHANGELOG.md +++ b/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. @@ -24,6 +27,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. ## [2.0.4] - 2022-12-13 ### Changed From 1810a4dbd14513b998923db4285b906d08a1e994 Mon Sep 17 00:00:00 2001 From: LPLafontaineB Date: Wed, 26 Apr 2023 10:59:28 -0400 Subject: [PATCH 6/6] fixing comment issue and adding clarification --- Assets/Tests/Runtime/ConnectionManagementTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Assets/Tests/Runtime/ConnectionManagementTests.cs b/Assets/Tests/Runtime/ConnectionManagementTests.cs index 008efbabf..e655f349a 100644 --- a/Assets/Tests/Runtime/ConnectionManagementTests.cs +++ b/Assets/Tests/Runtime/ConnectionManagementTests.cs @@ -411,7 +411,6 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect() var maxNbReconnectionAttempts = 0; - // Waiting for clients to fail to automatically reconnect for (var i = 0; i < NumberOfClients; i++) { var nbReconnectionAttempts = m_ClientConnectionManagers[i].NbReconnectAttempts; @@ -423,6 +422,7 @@ public IEnumerator UnexpectedServerShutdown_ClientsFailToReconnect() } } + // Waiting for clients to fail to automatically reconnect. We wait once for each reconnection attempt. for (var i = 0; i < maxNbReconnectionAttempts; i++) { yield return WaitForClientsConnectedOrTimeOut();