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>

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

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);

Assets/Mirror/Runtime/NetworkConnectionToClient.cs

@@ -6,9 +6,7 @@ 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);
 
@@ -16,6 +14,11 @@ public NetworkConnectionToClient(int networkConnectionId) : base(networkConnecti
         // 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));

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);
 

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)
             {

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()
         {

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);
+        }
     }
 }

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`.