fix: Fixes networking issues (#1958)

### Summary
- Puts back `EventSink.SocketConnect`.
- Reverts networking change to push the networking to a separate thread.
- Reverts changes to the firewall by removing the firewall queue.
- Fixes listeners not shutting down with the server.
- Fixes race condition causing connections to get stuck even after they are disposed.

> [!NOTE]
> **Developer Note**
> Networking has been reverted back to using the main thread instead of a background thread. This alleviated complexity and the requirement for concurrent queues all over the place.

Projects/Server/Events/SocketConnectionEvent.cs

@@ -0,0 +1,41 @@
+/*************************************************************************
+ * ModernUO                                                              *
+ * Copyright 2019-2023 - ModernUO Development Team                       *
+ * Email: [email protected]                                                *
+ * File: SocketConnectionEvent.cs                                        *
+ *                                                                       *
+ * This program is free software: you can redistribute it and/or modify  *
+ * it under the terms of the GNU General Public License as published by  *
+ * the Free Software Foundation, either version 3 of the License, or     *
+ * (at your option) any later version.                                   *
+ *                                                                       *
+ * You should have received a copy of the GNU General Public License     *
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>. *
+ *************************************************************************/
+
+using System;
+using System.Net.Sockets;
+using System.Runtime.CompilerServices;
+
+namespace Server;
+
+public class SocketConnectEventArgs
+{
+    public SocketConnectEventArgs(Socket c)
+    {
+        Connection = c;
+        AllowConnection = true;
+    }
+
+    public Socket Connection { get; }
+
+    public bool AllowConnection { get; set; }
+}
+
+public static partial class EventSink
+{
+    public static event Action<SocketConnectEventArgs> SocketConnect;
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public static void InvokeSocketConnect(SocketConnectEventArgs e) => SocketConnect?.Invoke(e);
+}

Projects/Server/Main.cs

@@ -336,6 +336,8 @@ private static void HandleClosed()
 
         World.WaitForWriteCompletion();
         World.ExitSerializationThreads();
+        PingServer.Shutdown();
+        TcpServer.Shutdown();
 
         if (!_crashed)
         {

Projects/Server/Network/Firewall/Firewall.cs

@@ -14,83 +14,21 @@
  *************************************************************************/
 
 using System;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Net;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
-using Server.Logging;
 
 namespace Server.Network;
 
 public static class Firewall
 {
-    private static readonly ILogger logger = LogFactory.GetLogger(typeof(Firewall));
-
     private static InternalValidationEntry _validationEntry;
     private static readonly Dictionary<IPAddress, bool> _isBlockedCache = new();
 
-    private static readonly ConcurrentQueue<(IFirewallEntry FirewallyEntry, bool Remove)> _firewallQueue = new();
     private static readonly SortedSet<IFirewallEntry> _firewallSet = new();
 
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public static IFirewallEntry RequestAddSingleIPEntry(string entry)
-    {
-        try
-        {
-            var firewallEntry = new SingleIpFirewallEntry(entry);
-            _firewallQueue.Enqueue((firewallEntry, false));
-            return firewallEntry;
-        }
-        catch (Exception e)
-        {
-            logger.Warning(e, "Failed to add firewall entry: {Pattern}", entry);
-            return null;
-        }
-    }
-
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public static IFirewallEntry RequestAddCIDREntry(string entry)
-    {
-        try
-        {
-            var firewallEntry = new CidrFirewallEntry(entry);
-            _firewallQueue.Enqueue((firewallEntry, false));
-            return firewallEntry;
-        }
-        catch (Exception e)
-        {
-            logger.Warning(e, "Failed to add firewall entry: {Pattern}", entry);
-            return null;
-        }
-    }
-
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public static void RequestAddEntry(IFirewallEntry entry)
-    {
-        _firewallQueue.Enqueue((entry, false));
-    }
-
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public static void RequestRemoveEntry(IFirewallEntry entry)
-    {
-        _firewallQueue.Enqueue((entry, true));
-    }
-
-    internal static void ProcessQueue()
-    {
-        while (_firewallQueue.TryDequeue(out var entry))
-        {
-            if (entry.Remove)
-            {
-                RemoveEntry(entry.FirewallyEntry);
-            }
-            else
-            {
-                AddEntry(entry.FirewallyEntry);
-            }
-        }
-    }
+    public static SortedSet<IFirewallEntry> FirewallSet => _firewallSet;
 
     internal static bool IsBlocked(IPAddress address)
     {
@@ -128,22 +66,32 @@ internal static bool IsBlocked(IPAddress address)
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    private static void AddEntry(IFirewallEntry firewallEntry)
+    public static bool Add(IFirewallEntry firewallEntry)
     {
-        _firewallSet.Add(firewallEntry);
-        _isBlockedCache.Clear();
+        if (_firewallSet.Add(firewallEntry))
+        {
+            _isBlockedCache.Clear();
+            return true;
+        }
+
+        return false;
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    private static void RemoveEntry(IFirewallEntry entry)
+    public static bool Remove(IFirewallEntry entry)
     {
         if (entry == null)
         {
-            return;
+            return false;
+        }
+
+        if (_firewallSet.Remove(entry))
+        {
+            _isBlockedCache.Clear();
+            return true;
         }
 
-        _firewallSet.Remove(entry);
-        _isBlockedCache.Clear();
+        return false;
     }
 
     private class InternalValidationEntry : BaseFirewallEntry

Projects/Server/Network/IPLimiter.cs

@@ -51,22 +51,32 @@ public static bool Verify(IPAddress ourAddress)
 
         var now = Core.Now;
 
-        CheckThrottledAddresses(now);
+        IPAccessLog accessLog;
+
+        while (_throttledAddresses.Count > 0)
+        {
+            accessLog = _throttledAddresses.Min;
+            if (now <= accessLog.Expiration)
+            {
+                break;
+            }
+
+            _throttledAddresses.Remove(accessLog);
+        }
 
         _accessCheck.IPAddress = ourAddress;
 
-        if (_connectionAttempts.TryGetValue(_accessCheck, out var accessLog))
+        if (_connectionAttempts.TryGetValue(_accessCheck, out accessLog))
         {
             _connectionAttempts.Remove(accessLog);
             accessLog.Count++;
+            accessLog.Expiration = now + ConnectionAttemptsDuration;
 
             if (now <= accessLog.Expiration && accessLog.Count >= MaxConnections)
             {
-                BlockConnection(now, accessLog);
+                _throttledAddresses.Add(accessLog);
                 return false;
             }
-
-            accessLog.Expiration = now + ConnectionAttemptsDuration;
         }
         else
         {
@@ -79,26 +89,6 @@ public static bool Verify(IPAddress ourAddress)
         return true;
     }
 
-    private static void BlockConnection(DateTime now, IPAccessLog accessLog)
-    {
-        accessLog.Expiration = now + ConnectionAttemptsDuration;
-        _throttledAddresses.Add(accessLog);
-    }
-
-    private static void CheckThrottledAddresses(DateTime now)
-    {
-        while (_throttledAddresses.Count > 0)
-        {
-            var accessLog = _throttledAddresses.Min;
-            if (now <= accessLog.Expiration)
-            {
-                break;
-            }
-
-            _throttledAddresses.Remove(accessLog);
-        }
-    }
-
     private class IPAccessLog : IComparable<IPAccessLog>
     {
         public IPAddress IPAddress;

Projects/Server/Network/NetState/NetState.cs

@@ -32,8 +32,6 @@
 
 namespace Server.Network;
 
-public delegate void NetStateCreatedCallback(NetState ns);
-
 public delegate void DecodePacket(Span<byte> buffer, ref int length);
 public delegate int EncodePacket(ReadOnlySpan<byte> inputBuffer, Span<byte> outputBuffer);
 
@@ -56,8 +54,6 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
     private static readonly Queue<NetState> _throttled = new(256);
     private static readonly Queue<NetState> _throttledPending = new(256);
 
-    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;
@@ -68,8 +64,8 @@ public partial class NetState : IComparable<NetState>, IValueLinkListNode<NetSta
     private volatile DecodePacket _packetDecoder;
     private volatile EncodePacket _packetEncoder;
     private bool _flushQueued;
-    private readonly long[] _packetThrottles = new long[0x100];
-    private readonly long[] _packetCounts = new long[0x100];
+    private long[] _packetThrottles;
+    private long[] _packetCounts;
     private string _disconnectReason = string.Empty;
 
     internal ParserState _parserState = ParserState.AwaitingNextPacket;
@@ -143,8 +139,12 @@ public NetState(Socket connection)
             _toString = "(error)";
         }
 
+        _instances.Add(this);
+        _connecting.Add(this);
         _handle = GCHandle.Alloc(this);
 
+        LogInfo($"Connected. [{_instances.Count} Online]");
+
         try
         {
             _pollGroup.Add(connection, _handle);
@@ -254,22 +254,30 @@ private void SetPacketTime(int packetID)
     {
         if (packetID is >= 0 and < 0x100)
         {
+            _packetThrottles ??= new long[0x100];
             _packetThrottles[packetID] = Core.TickCount;
         }
     }
 
-    public long GetPacketTime(int packetID) => packetID is >= 0 and < 0x100 ? _packetThrottles[packetID] : 0;
+    public long GetPacketTime(int packetID) =>
+        packetID is >= 0 and < 0x100 && _packetThrottles != null ? _packetThrottles[packetID] : 0;
 
     private void UpdatePacketCount(int packetID)
     {
         if (packetID is >= 0 and < 0x100)
         {
+            _packetCounts ??= new long[0x100];
             _packetCounts[packetID]++;
         }
     }
 
     public int CheckPacketCounts()
     {
+        if (_packetCounts == null)
+        {
+            return 0;
+        }
+
         for (int i = 0; i < _packetCounts.Length; i++)
         {
             long count = _packetCounts[i];
@@ -917,7 +925,7 @@ private static void DisconnectUnattachedSockets()
             var ns = _connecting.Min;
             var socketTime = ns.ConnectedOn;
 
-            // If the socket has been connected for less than 2 seconds, we can stop checking
+            // If the socket has been connected for less than the limit, we can stop checking
             if (now - socketTime < ConnectingSocketIdleLimit)
             {
                 break;
@@ -946,17 +954,6 @@ public static void Slice()
     {
         DisconnectUnattachedSockets();
 
-        const int maxEntriesPerLoop = 32;
-        var count = 0;
-        while (++count <= maxEntriesPerLoop && TcpServer.ConnectedQueue.TryDequeue(out var ns))
-        {
-            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]");
-        }
-
         while (_throttled.Count > 0)
         {
             var ns = _throttled.Dequeue();
@@ -972,7 +969,7 @@ public static void Slice()
             _throttled.Enqueue(_throttledPending.Dequeue());
         }
 
-        count = _pollGroup.Poll(_polledStates);
+        var count = _pollGroup.Poll(_polledStates);
 
         if (count > 0)
         {

Projects/Server/Network/PingServer.cs

@@ -16,7 +16,6 @@
 using System.Collections.Generic;
 using System.Net;
 using System.Net.Sockets;
-using System.Threading;
 using Server.Logging;
 
 namespace Server.Network;
@@ -47,8 +46,8 @@ public static void Start()
             return;
         }
 
-        HashSet<IPEndPoint> listeningAddresses = new HashSet<IPEndPoint>();
-        List<UdpClient> listeners = new List<UdpClient>();
+        HashSet<IPEndPoint> listeningAddresses = [];
+        List<UdpClient> listeners = [];
 
         foreach (var serverIpep in ServerConfiguration.Listeners)
         {
@@ -70,7 +69,7 @@ public static void Start()
             }
 
             listeners.Add(listener);
-            new Thread(BeginAcceptingUdpRequest).Start(listener);
+            BeginAcceptingUdpRequest(listener);
         }
 
         foreach (var ipep in listeningAddresses)
@@ -140,4 +139,12 @@ private static async void BeginAcceptingUdpRequest(object state)
             }
         }
     }
+
+    public static void Shutdown()
+    {
+        foreach (var listener in Listeners)
+        {
+            listener.Close();
+        }
+    }
 }