feat: Disconnect Dead Clients (#1724)

* feat: Disconnect Dead Clients * Moved code to NetworkConnectionToClient * Fixed type * WIP * Trying to solve the mystery of the Host * Updated Example * fixed comment whitespace * Final Cleanup and Unit Test * Removed extra warning * Reverted change to scene file * Changed to Play test, still not working * Added NetworkServer.Update time loop * Removed commented code * fixed comment * Filled in ServerDisconnect so it behaves as expected. * fixed comment * Renamed to bool IsClientAlive * Should be greater than * Added override that shouldn't be necessary. * changed asserts per Paul * Flipped < back * Shortened test time * Corrected comment * Lost the 1 * Updated NetworkServerTest * Update Assets/Mirror/Tests/Runtime/NetworkServerTest.cs * Added bool checkInactiveConnections * Tests for sync dictionary and sync set (#1753) * sync dictionary tests * rename * changing error message * sync set tests * silence unused method warning * test class name matches file name * test class name matches file name * test class name matches file name * test class name matches file name * test class name matches file name * test class name matches file name * test class name matches file name * Scope syncdict tests to their classes * Scope synclists tests to their classes * Scope syncset tests to their classes * test class name matches file name * Scope sample classes * test class name matches file name * fix: call the virtual OnRoomServerDisconnect before the base * fixed name * doc: Updated docs * Moved serverIdleTimeout to NetworkManager * fixed test to enable disconnectInactiveConnections * Copied disconnectInactiveConnections & serverIdleTimeout to NetworkServer Co-authored-by: James Frowen <jamesfrowendev@gmail.com> Co-authored-by: Paul Pacheco <paulpach@gmail.com>
MirageNet · Apr 23, 2020 · a2eb666 · a2eb666
1 parent c1bacc3
commit a2eb666
Show file tree

Hide file tree

Showing 10 changed files with 122 additions and 13 deletions.
diff --git a/Assets/Mirror/Runtime/LocalConnections.cs b/Assets/Mirror/Runtime/LocalConnections.cs
@@ -9,9 +9,7 @@ class ULocalConnectionToClient : NetworkConnectionToClient
     {
         internal ULocalConnectionToServer connectionToServer;
 
-        public ULocalConnectionToClient() : base(0)
-        {
-        }
+        public ULocalConnectionToClient() : base(0) { }
 
         public override string address => "localhost";
 
@@ -22,6 +20,9 @@ internal override bool Send(ArraySegment<byte> segment, int channelId = Channels
             return true;
         }
 
+        // override for host client: always return true.
+        internal override bool IsClientAlive() => true;
+
         internal void DisconnectInternal()
         {
             // set not ready and handle clientscene disconnect in any case

diff --git a/Assets/Mirror/Runtime/NetworkConnection.cs b/Assets/Mirror/Runtime/NetworkConnection.cs
@@ -86,14 +86,12 @@ public abstract class NetworkConnection : IDisposable
         public bool logNetworkMessages;
 
         /// <summary>
-        /// Creates a new NetworkConnection with the specified address
+        /// Creates a new NetworkConnection
         /// </summary>
-        internal NetworkConnection()
-        {
-        }
+        internal NetworkConnection() { }
 
         /// <summary>
-        /// Creates a new NetworkConnection with the specified address and connectionId
+        /// Creates a new NetworkConnection with the specified connectionId
         /// </summary>
         /// <param name="networkConnectionId"></param>
         internal NetworkConnection(int networkConnectionId)
@@ -282,6 +280,17 @@ internal void TransportReceive(ArraySegment<byte> buffer, int channelId)
             }
         }
 
+        // Failsafe to kick clients that have stopped sending anything to the server.
+        // Clients Ping the server every 2 seconds but transports are unreliable
+        // when it comes to properly generating Disconnect messages to the server.
+        // This cannot be abstract because then NetworkConnectionToServer
+        // would require and override that would never be called
+        // This is overriden in NetworkConnectionToClient.
+        internal virtual bool IsClientAlive()
+        {
+            return true;
+        }
+
         internal void AddOwnedObject(NetworkIdentity obj)
         {
             clientOwnedObjects.Add(obj);

diff --git a/Assets/Mirror/Runtime/NetworkConnectionToClient.cs b/Assets/Mirror/Runtime/NetworkConnectionToClient.cs
@@ -6,16 +6,19 @@ namespace Mirror
 {
     public class NetworkConnectionToClient : NetworkConnection
     {
-        public NetworkConnectionToClient(int networkConnectionId) : base(networkConnectionId)
-        {
-        }
+        public NetworkConnectionToClient(int networkConnectionId) : base(networkConnectionId) { }
 
         public override string address => Transport.activeTransport.ServerGetClientAddress(connectionId);
 
         // internal because no one except Mirror should send bytes directly to
         // the client. they would be detected as a message. send messages instead.
         readonly List<int> singleConnectionId = new List<int> { -1 };
 
+        // Failsafe to kick clients that have stopped sending anything to the server.
+        // Clients ping the server every 2 seconds but transports are unreliable
+        // when it comes to properly generating Disconnect messages to the server.
+        internal override bool IsClientAlive() => Time.time - lastMessageTime < NetworkServer.serverIdleTimeout;
+
         internal override bool Send(ArraySegment<byte> segment, int channelId = Channels.DefaultReliable)
         {
             if (logNetworkMessages) Debug.Log("ConnectionSend " + this + " bytes:" + BitConverter.ToString(segment.Array, segment.Offset, segment.Count));

diff --git a/Assets/Mirror/Runtime/NetworkManager.cs b/Assets/Mirror/Runtime/NetworkManager.cs
@@ -101,6 +101,23 @@ public class NetworkManager : MonoBehaviour
         [Tooltip("Maximum number of concurrent connections.")]
         public int maxConnections = 4;
 
+        // This value is passed to NetworkServer in SetupServer
+        /// <summary>
+        /// Should the server disconnect remote connections that have gone silent for more than Server Idle Timeout?
+        /// </summary>
+        [Tooltip("Server Only - Disconnects remote connections that have been silent for more than Server Idle Timeout")]
+        public bool disconnectInactiveConnections;
+
+        // This value is passed to NetworkServer in SetupServer
+        /// <summary>
+        /// Timeout in seconds since last message from a client after which server will auto-disconnect.
+        /// <para>By default, clients send at least a Ping message every 2 seconds.</para>
+        /// <para>The Host client is immune from idle timeout disconnection.</para>
+        /// <para>Default value is 60 seconds.</para>
+        /// </summary>
+        [Tooltip("Timeout in seconds since last message from a client after which server will auto-disconnect if Disconnect Inactive Connections is enabled.")]
+        public float serverIdleTimeout = 60f;
+
         [Header("Authentication")]
         [Tooltip("Authentication component attached to this object")]
         public NetworkAuthenticator authenticator;
@@ -290,6 +307,10 @@ void SetupServer()
 
             ConfigureServerFrameRate();
 
+            // Copy auto-disconnect settings to NetworkServer
+            NetworkServer.serverIdleTimeout = serverIdleTimeout;
+            NetworkServer.disconnectInactiveConnections = disconnectInactiveConnections;
+
             // start listening to network connections
             NetworkServer.Listen(maxConnections);
 

diff --git a/Assets/Mirror/Runtime/NetworkServer.cs b/Assets/Mirror/Runtime/NetworkServer.cs
@@ -56,6 +56,21 @@ public static class NetworkServer
         /// </summary>
         public static bool active { get; private set; }
 
+        /// <summary>
+        /// Should the server disconnect remote connections that have gone silent for more than Server Idle Timeout?
+        /// <para>This value is initially set from NetworkManager in SetupServer and can be changed at runtime</para>
+        /// </summary>
+        public static bool disconnectInactiveConnections;
+
+        /// <summary>
+        /// Timeout in seconds since last message from a client after which server will auto-disconnect.
+        /// <para>This value is initially set from NetworkManager in SetupServer and can be changed at runtime</para>
+        /// <para>By default, clients send at least a Ping message every 2 seconds.</para>
+        /// <para>The Host client is immune from idle timeout disconnection.</para>
+        /// <para>Default value is 60 seconds.</para>
+        /// </summary>
+        public static float serverIdleTimeout = 60f;
+
         // cache the Send(connectionIds) list to avoid allocating each time
         static readonly List<int> connectionIdsCache = new List<int>();
 
@@ -405,6 +420,20 @@ internal static void Update()
             if (!active)
                 return;
 
+            // Check for dead clients but exclude the host client because it
+            // doesn't ping itself and therefore may appear inactive.
+            if (disconnectInactiveConnections)
+            {
+                foreach (NetworkConnectionToClient conn in connections.Values)
+                {
+                    if (!conn.IsClientAlive())
+                    {
+                        Debug.LogWarning($"Disconnecting {conn} for inactivity!");
+                        conn.Disconnect();
+                    }
+                }
+            }
+
             // update all server objects
             foreach (KeyValuePair<uint, NetworkIdentity> kvp in NetworkIdentity.spawned)
             {

diff --git a/Assets/Mirror/Tests/Common/MemoryTransport.cs b/Assets/Mirror/Tests/Common/MemoryTransport.cs
@@ -125,7 +125,26 @@ public override bool ServerSend(List<int> connectionIds, int channelId, ArraySeg
             }
             return false;
         }
-        public override bool ServerDisconnect(int connectionId) => false;
+
+        public override bool ServerDisconnect(int connectionId)
+        {
+            // clear all pending messages that we may have received.
+            // over the wire, we wouldn't receive any more pending messages
+            // ether after calling disconnect.
+            serverIncoming.Clear();
+
+            // add client disconnected message with connectionId
+            clientIncoming.Enqueue(new Message(connectionId, EventType.Disconnected, null));
+
+            // add server disconnected message with connectionId
+            serverIncoming.Enqueue(new Message(connectionId, EventType.Disconnected, null));
+
+            // not active anymore
+            serverActive = false;
+
+            return false;
+        }
+
         public override string ServerGetClientAddress(int connectionId) => string.Empty;
         public override void ServerStop()
         {

diff --git a/Assets/Mirror/Tests/Runtime/NetworkServerTest.cs b/Assets/Mirror/Tests/Runtime/NetworkServerTest.cs
@@ -23,5 +23,31 @@ public IEnumerator DestroyPlayerForConnectionTest()
 
             Assert.That(player == null, "Player should be destroyed with DestroyPlayerForConnection");
         }
+
+        [UnityTest]
+        public IEnumerator DisconnectTimeoutTest()
+        {
+            // Set high ping frequency so no NetworkPingMessage is generated
+            NetworkTime.PingFrequency = 5f;
+
+            // Set a short timeout for this test and enable disconnectInactiveConnections
+            NetworkServer.serverIdleTimeout = 1f;
+            NetworkServer.disconnectInactiveConnections = true;
+
+            GameObject remotePlayer = new GameObject("RemotePlayer", typeof(NetworkIdentity));
+            NetworkConnectionToClient remoteConnection = new NetworkConnectionToClient(1);
+            NetworkServer.OnConnected(remoteConnection);
+            NetworkServer.AddPlayerForConnection(remoteConnection, remotePlayer);
+
+            // There's a host player from HostSetup + remotePlayer
+            Assert.That(NetworkServer.connections.Count, Is.EqualTo(2));
+
+            // wait 2 seconds for remoteConnection to timeout as idle
+            yield return new WaitForSeconds(2f);
+
+            // host client connection should still be alive
+            Assert.That(NetworkServer.connections.Count, Is.EqualTo(1));
+            Assert.That(NetworkServer.localConnection, Is.Not.Null);
+        }
     }
 }
diff --git a/doc/Components/NetworkManagerInspector.png b/doc/Components/NetworkManagerInspector.png
diff --git a/doc/Components/NetworkRoomManager.png b/doc/Components/NetworkRoomManager.png
diff --git a/doc/General/ChangeLog.md b/doc/General/ChangeLog.md
@@ -1,6 +1,6 @@
 # Change Log
 
-**Mirror is published to the Asset Store at the start of every month, unless some critical issue causes a delay.**
+**Mirror is published to the [Asset Store](https://assetstore.unity.com/packages/tools/network/mirror-129321) at the start of every month, unless some critical issue causes a delay.**
 
 Mirror uses semantic versioning, and the versions shown here are those that were published to the Asset Store, and occasionally major version bumps happen mid-month between store submissions and are therefore not individually shown here.
 
@@ -9,6 +9,7 @@ Mirror uses semantic versioning, and the versions shown here are those that were
 - Added: [SyncLists](../Guides/Sync/SyncLists.md) now have Find and FindAll functions.
 - Added: NetworkBehaviour now has OnStopServer and OnStopClient virtual methods
 - Added: Weaver now supports custom Reader & Writer for types in other assemblies
+- Added: Network Manager now has an optional setting to check for and disconnect remote connections that have gone silent for a specified interval.
 - Fixed: NetworkAnimator no longer double-fires SetTrigger / ResetTrigger on the host client
 - Fixed: Destroy is no longer invoked twice on the server for the player object.
 - Changed: NetworkBehaviour: `OnNetworkDestroy` was renamed to `OnStopClient`.