From 183f6fa4acd0386c8b27aafdbf8d071f35635961 Mon Sep 17 00:00:00 2001
From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com>
Date: Wed, 3 Apr 2024 08:14:41 -0700
Subject: [PATCH] fix: Fixes stuck connections (#1708)

---
 Projects/Server/Main.cs                       |   2 +-
 Projects/Server/Network/Firewall/Firewall.cs  |   9 +-
 Projects/Server/Network/NetState/NetState.cs  | 113 ++++++++++++++----
 Projects/Server/Network/PingServer.cs         |  52 ++------
 Projects/Server/Network/TcpServer.cs          |  70 +++++------
 .../Network/Packets/IncomingAccountPackets.cs |   3 +-
 Projects/UOContent/Network/UOGateway.cs       |   7 +-
 7 files changed, 136 insertions(+), 120 deletions(-)

diff --git a/Projects/Server/Main.cs b/Projects/Server/Main.cs
index d17c45108..13976d423 100644
--- a/Projects/Server/Main.cs
+++ b/Projects/Server/Main.cs
@@ -565,7 +565,7 @@ public static class Core
 
                 // Handle networking
                 NetState.Slice();
-                PingServer.Slice();
+                // PingServer.Slice();
 
                 // Execute captured post-await methods (like Timer.Pause)
                 LoopContext.ExecuteTasks();
diff --git a/Projects/Server/Network/Firewall/Firewall.cs b/Projects/Server/Network/Firewall/Firewall.cs
index 5a261488b..f35046d40 100644
--- a/Projects/Server/Network/Firewall/Firewall.cs
+++ b/Projects/Server/Network/Firewall/Firewall.cs
@@ -134,13 +134,16 @@ public static class Firewall
         _isBlockedCache.Clear();
     }
 
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
     private static void RemoveEntry(IFirewallEntry entry)
     {
-        if (entry != null)
+        if (entry == null)
         {
-            _firewallSet.Remove(entry);
-            _isBlockedCache.Clear();
+            return;
         }
+
+        _firewallSet.Remove(entry);
+        _isBlockedCache.Clear();
     }
 
     private class InternalValidationEntry : BaseFirewallEntry
diff --git a/Projects/Server/Network/NetState/NetState.cs b/Projects/Server/Network/NetState/NetState.cs
index a52fbfc07..19208647e 100755
--- a/Projects/Server/Network/NetState/NetState.cs
+++ b/Projects/Server/Network/NetState/NetState.cs
@@ -43,6 +43,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
 {
     private static readonly ILogger logger = LogFactory.GetLogger(typeof(NetState));
 
+    private static readonly TimeSpan ConnectingSocketIdleLimit = TimeSpan.FromMilliseconds(2000); // 2 seconds
     private const int RecvPipeSize = 1024 * 64;
     private const int SendPipeSize = 1024 * 256;
     private const int GumpCap = 512;
@@ -60,6 +61,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
 
     public static NetStateCreatedCallback CreatedCallback { get; set; }
 
+    private static readonly SortedSet<NetState> _connecting = new(NetStateConnectingComparer.Instance);
     private static readonly HashSet<NetState> _instances = new(2048);
     public static IReadOnlySet<NetState> Instances => _instances;
 
@@ -84,6 +86,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
     // Speed Hack Prevention
     internal long _movementCredit;
     internal long _nextMovementTime;
+    private IAccount _account;
 
     internal enum ParserState
     {
@@ -234,7 +237,19 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
 
     public ServerInfo[] ServerInfo { get; set; }
 
-    public IAccount Account { get; set; }
+    public IAccount Account
+    {
+        get => _account;
+        set
+        {
+            if (_account != null)
+            {
+                _connecting.Remove(this);
+            }
+
+            _account = value;
+        }
+    }
 
     public string Assistant { get; set; }
 
@@ -690,7 +705,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
                         case ProtocolState.LoginServer_ServerSelectAck:
                             {
 #if STRICT_UO_PROTOCOL
-                                    HandleError(packetId, packetLength);
+                                HandleError(packetId, packetLength);
 #else
                                 // Reset the state because CUO/Orion do not reconnect
                                 _parserState = ParserState.AwaitingNextPacket;
@@ -867,6 +882,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
     {
         _flushQueued = false;
 
+        // We don't have a running check since we need to send the last bits of data even after a disconnect, but before a dispose.
         if (Connection == null)
         {
             return true;
@@ -892,12 +908,14 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
             {
                 logger.Debug(ex, "Disconnected due to a socket exception");
                 Disconnect(string.Empty);
+                return true;
             }
         }
         catch (Exception ex)
         {
             Disconnect($"Disconnected with error: {ex}");
             TraceException(ex);
+            return true;
         }
 
         if (bytesWritten > 0)
@@ -957,6 +975,33 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
         _nextActivityCheck = Core.TickCount + 90000;
     }
 
+    private static void DisconnectUnattachedSockets()
+    {
+        var now = Core.Now;
+
+        // Clear out any sockets that have been connecting for too long
+        while (_connecting.Count > 0)
+        {
+            var ns = _connecting.Min;
+            var socketTime = ns.ConnectedOn;
+
+            // If the socket has been connected for less than 2 seconds, we can stop checking
+            if (now - socketTime < ConnectingSocketIdleLimit)
+            {
+                break;
+            }
+
+            // Socket must have finished the entire authentication process or be forcibly disconnected.
+            if (!ns.Running || !ns.SentFirstPacket || !ns.Seeded || ns.Account == null)
+            {
+                // Not sending a message because it will fill up the logs.
+                ns.Disconnect(null);
+            }
+
+            _connecting.Remove(ns);
+        }
+    }
+
     public static void FlushAll()
     {
         while (_flushPending.Count != 0)
@@ -967,6 +1012,8 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
 
     public static void Slice()
     {
+        DisconnectUnattachedSockets();
+
         const int maxEntriesPerLoop = 32;
         var count = 0;
         while (++count <= maxEntriesPerLoop && TcpServer.ConnectedQueue.TryDequeue(out var ns))
@@ -974,6 +1021,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
             CreatedCallback?.Invoke(ns);
 
             _instances.Add(ns);
+            _connecting.Add(ns); // Add to the connecting set, and remove them when they authenticated.
             ns.LogInfo($"Connected. [{Instances.Count} Online]");
         }
 
@@ -1111,17 +1159,6 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
 
         _running = false;
 
-#if THREADGUARD
-            if (Thread.CurrentThread != Core.Thread)
-            {
-                Utility.PushColor(ConsoleColor.Red);
-                Console.WriteLine("Attempting to disconnect a netstate from an invalid thread!");
-                Console.WriteLine(new StackTrace());
-                Utility.PopColor();
-                return;
-            }
-#endif
-
         _disconnectReason = reason;
         _disposed.Enqueue(this);
     }
@@ -1165,17 +1202,6 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
             throw new Exception("Disconnected a NetState that is still running.");
         }
 
-#if THREADGUARD
-            if (Thread.CurrentThread != Core.Thread)
-            {
-                Utility.PushColor(ConsoleColor.Red);
-                Console.WriteLine("Attempting to dispose a netstate from an invalid thread!");
-                Console.WriteLine(new StackTrace());
-                Utility.PopColor();
-                return;
-            }
-#endif
-
         var m = Mobile;
         if (m?.NetState == this)
         {
@@ -1183,6 +1209,7 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
         }
 
         _instances.Remove(this);
+        _connecting.Remove(this);
 
         try
         {
@@ -1210,8 +1237,44 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
         CityInfo = null;
         Connection = null;
 
-        var count = Instances.Count;
+        var count = _instances.Count;
 
         LogInfo(a != null ? $"Disconnected. [{count} Online] [{a}]" : $"Disconnected. [{count} Online]");
     }
+
+    private class NetStateConnectingComparer : IComparer<NetState>
+    {
+        public static readonly IComparer<NetState> Instance = new NetStateConnectingComparer();
+
+        public int Compare(NetState x, NetState y)
+        {
+            if (x == null && y == null)
+            {
+                return 0;
+            }
+
+            if (x == null)
+            {
+                return -1;
+            }
+
+            if (y == null)
+            {
+                return 1;
+            }
+
+            if (ReferenceEquals(x, y))
+            {
+                return 0;
+            }
+
+            var connectedOn = x.ConnectedOn.CompareTo(y.ConnectedOn);
+            if (connectedOn != 0)
+            {
+                return connectedOn;
+            }
+
+            return x.CompareTo(y);
+        }
+    }
 }
diff --git a/Projects/Server/Network/PingServer.cs b/Projects/Server/Network/PingServer.cs
index 3142f81f4..31db1ad08 100644
--- a/Projects/Server/Network/PingServer.cs
+++ b/Projects/Server/Network/PingServer.cs
@@ -13,11 +13,10 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>. *
  *************************************************************************/
 
-using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Net;
 using System.Net.Sockets;
-using System.Threading.Tasks;
+using System.Threading;
 using Server.Logging;
 
 namespace Server.Network;
@@ -26,12 +25,8 @@ public static class PingServer
 {
     private static readonly ILogger logger = LogFactory.GetLogger(typeof(PingServer));
 
-    private const int MaxConnectionsPerLoop = 128;
-
     public static int MaxQueued { get; set; }
 
-    private static readonly ConcurrentQueue<(UdpClient, UdpReceiveResult)> _udpResponseQueue = new();
-
     public static UdpClient[] Listeners { get; private set; }
 
     public static bool Enabled { get; private set; }
@@ -57,7 +52,6 @@ public static class PingServer
 
         foreach (var serverIpep in ServerConfiguration.Listeners)
         {
-            var cancellationToken = Core.ClosingTokenSource.Token;
             var ipep = new IPEndPoint(serverIpep.Address, Port);
 
             var listener = CreateListener(ipep);
@@ -76,7 +70,7 @@ public static class PingServer
             }
 
             listeners.Add(listener);
-            Task.Run(() => BeginAcceptingUdpRequest(listener), cancellationToken).ConfigureAwait(false);
+            new Thread(BeginAcceptingUdpRequest).Start(listener);
         }
 
         foreach (var ipep in listeningAddresses)
@@ -87,22 +81,6 @@ public static class PingServer
         Listeners = listeners.ToArray();
     }
 
-    public static void Slice()
-    {
-        if (!Enabled)
-        {
-            return;
-        }
-
-        int count = 0;
-
-        while (++count <= MaxConnectionsPerLoop && _udpResponseQueue.TryDequeue(out var udpTuple))
-        {
-            var (listener, result) = udpTuple;
-            SendResponse(listener, result.Buffer, result.RemoteEndPoint);
-        }
-    }
-
     public static UdpClient CreateListener(IPEndPoint ipep)
     {
         var listener = new Socket(ipep.AddressFamily, SocketType.Dgram, ProtocolType.Udp)
@@ -140,8 +118,13 @@ public static class PingServer
         return null;
     }
 
-    private static async void BeginAcceptingUdpRequest(UdpClient listener)
+    private static async void BeginAcceptingUdpRequest(object state)
     {
+        if (state is not UdpClient listener)
+        {
+            return;
+        }
+
         var cancellationToken = Core.ClosingTokenSource.Token;
 
         while (!cancellationToken.IsCancellationRequested)
@@ -149,12 +132,7 @@ public static class PingServer
             try
             {
                 var result = await listener.ReceiveAsync(cancellationToken);
-
-                if (_udpResponseQueue.Count < MaxQueued)
-                {
-                    _udpResponseQueue.Enqueue((listener, result));
-                }
-
+                await listener.SendAsync(result.Buffer, result.RemoteEndPoint, cancellationToken);
             }
             catch
             {
@@ -162,16 +140,4 @@ public static class PingServer
             }
         }
     }
-
-    private static async Task SendResponse(UdpClient listener, byte[] data, IPEndPoint ipep)
-    {
-        try
-        {
-            await listener.SendAsync(data, ipep, Core.ClosingTokenSource.Token);
-        }
-        catch
-        {
-            // ignored
-        }
-    }
 }
diff --git a/Projects/Server/Network/TcpServer.cs b/Projects/Server/Network/TcpServer.cs
index 502fd63f3..892c4f589 100644
--- a/Projects/Server/Network/TcpServer.cs
+++ b/Projects/Server/Network/TcpServer.cs
@@ -1,6 +1,6 @@
 /*************************************************************************
  * ModernUO                                                              *
- * Copyright 2019-2023 - ModernUO Development Team                       *
+ * Copyright 2019-2024 - ModernUO Development Team                       *
  * Email: hi@modernuo.com                                                *
  * File: TcpServer.cs                                                    *
  *                                                                       *
@@ -23,7 +23,6 @@ using System.Net.NetworkInformation;
 using System.Net.Sockets;
 using System.Runtime.CompilerServices;
 using System.Threading;
-using System.Threading.Tasks;
 using Server.Logging;
 using Server.Misc;
 
@@ -33,7 +32,6 @@ public static class TcpServer
 {
     private static readonly ILogger logger = LogFactory.GetLogger(typeof(TcpServer));
 
-    private const long MaximumSocketIdleDelay = 2000; // 2 seconds
     private const long ListenerErrorMessageDelay = 10000; // 10 seconds
 
     private static long _nextMaximumSocketsReachedMessage;
@@ -47,9 +45,6 @@ public static class TcpServer
     public static IPEndPoint[] ListeningAddresses { get; private set; }
     public static Socket[] Listeners { get; private set; }
 
-    // By default should sort T1 then T2
-    public static readonly SortedSet<(long ConnectedAt, NetState NetState)> _socketsConnecting = [];
-
     public static ConcurrentQueue<NetState> ConnectedQueue { get; } = [];
 
     public static void Configure()
@@ -109,28 +104,34 @@ public static class TcpServer
         return null;
     }
 
-    private static async Task BeginAcceptingSockets(Socket listener)
+    private static async void BeginAcceptingSockets(object state)
     {
+        if (state is not Socket listener)
+        {
+            return;
+        }
+
         var cancellationToken = Core.ClosingTokenSource.Token;
 
-        try
+        while (!cancellationToken.IsCancellationRequested)
         {
-            var socket = await listener.AcceptAsync(cancellationToken);
-            _connectingQueue.Enqueue(socket);
-            _queueSemaphore.Release();
-        }
-        catch(OperationCanceledException)
-        {
-            return;
+            try
+            {
+                var socket = await listener.AcceptAsync(cancellationToken);
+                _connectingQueue.Enqueue(socket);
+                _queueSemaphore.Release();
+            }
+            catch (OperationCanceledException)
+            {
+                return;
+            }
+            catch
+            {
+                // ignored
+            }
         }
 
-        if (cancellationToken.IsCancellationRequested)
-        {
-            listener.Close();
-            return;
-        }
-
-        Task.Run(() => BeginAcceptingSockets(listener), cancellationToken).ConfigureAwait(false);
+        listener.Close();
     }
 
     private static void ProcessConnections()
@@ -164,7 +165,7 @@ public static class TcpServer
             {
                 listeners.Add(listener);
 
-                Task.Run(() => BeginAcceptingSockets(listener), cancellationToken).ConfigureAwait(false);
+                new Thread(BeginAcceptingSockets).Start(listener);
             }
         }
 
@@ -198,6 +199,10 @@ public static class TcpServer
             {
                 return;
             }
+            catch (Exception e)
+            {
+                logger.Error(e, "Error occurred in ProcessConnections");
+            }
         }
     }
 
@@ -221,24 +226,6 @@ public static class TcpServer
         var ipLimiter = IPLimiter.Enabled;
         try
         {
-            // Clear out any sockets that have been connecting for too long
-            while (_socketsConnecting.Count > 0)
-            {
-                var socketTime = _socketsConnecting.Min; // Earliest connected socket
-                if (Core.TickCount - socketTime.ConnectedAt <= MaximumSocketIdleDelay)
-                {
-                    break;
-                }
-
-                var socketToCheck = socketTime.NetState;
-                if (socketToCheck.Running && !socketToCheck.Seeded)
-                {
-                    socketToCheck.Disconnect(null);
-                }
-
-                _socketsConnecting.Remove(socketTime);
-            }
-
             var remoteIP = ((IPEndPoint)socket.RemoteEndPoint)!.Address;
 
             if (NetState.Instances.Count >= MaxConnections)
@@ -279,7 +266,6 @@ public static class TcpServer
             }
 
             var ns = new NetState(socket);
-            _socketsConnecting.Add((Core.TickCount, ns));
             ConnectedQueue.Enqueue(ns);
         }
         catch
diff --git a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
index 6725549a3..f77e5ba30 100644
--- a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
+++ b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
@@ -442,8 +442,6 @@ public static class IncomingAccountPackets
 
     public static void AccountLogin(NetState state, SpanReader reader)
     {
-        // TODO: Throttle Connection
-
         if (state.SentFirstPacket)
         {
             state.Disconnect("Duplicate account login packet sent.");
@@ -478,6 +476,7 @@ public static class IncomingAccountPackets
         }
         else
         {
+            state.Account = null;
             AccountLogin_ReplyRej(state, accountLoginEventArgs.RejectReason);
         }
     }
diff --git a/Projects/UOContent/Network/UOGateway.cs b/Projects/UOContent/Network/UOGateway.cs
index 62970c0ac..8d0c5e7e7 100644
--- a/Projects/UOContent/Network/UOGateway.cs
+++ b/Projects/UOContent/Network/UOGateway.cs
@@ -88,13 +88,12 @@ namespace Server.Network
             }
 
             var str =
-                $"ModernUO, Name={name}, Age={age}, Clients={clients}, Items={items}, Chars={mobiles}, Mem={mem}K, Ver=2";
+                $"ModernUO, Name={name}, Age={age}, Clients={clients}, Items={items}, Chars={mobiles}, Mem={mem}K, Ver=2\0";
 
-            var length = Encoding.UTF8.GetMaxByteCount(str.Length);
+            var length = Encoding.UTF8.GetByteCount(str);
 
-            Span<byte> span = stackalloc byte[length + 1];
+            Span<byte> span = stackalloc byte[length];
             Encoding.UTF8.GetBytes(str, span);
-            span[^1] = 0; // Terminator
 
             ns.Send(span);
         }