perf: stop the auto-denylist re-sweeping for every rejected address
At the cap, Hold() swept the whole dictionary before rejecting each new
distinct address. Entries are held 15 minutes, so during a sustained
distinct-source flood -- the case the cap exists for -- almost every one
of those sweeps freed nothing and the next address swept again. _warnedFull
suppressed the log, not the work.
Measured on the same loop shape (Dictionary<UInt128,long>, iterate, Remove
in place, Release, best of 20):
entries 0% lapsed 50% 100%
1,024 0.001ms 0.003 0.005
8,192 0.006ms 0.060 0.095
65,536 0.112ms 0.735 1.053
262,144 0.558ms 4.116 5.233
At the shipped 65,536 cap that is 0.11ms per rejected address: 11ms/sec of
wasted loop at 100 new addresses/sec, 110ms/sec at 1000. The bound became a
cost multiplier under exactly the load it bounds.
A sweep that just ran cannot have freed more, so cap-triggered sweeps are
now throttled to one a second, taking the worst case to 0.11ms/sec. The
periodic sweep is untouched and was never the problem: bounded by the cap,
allocation-free, early-outs when empty, ~1ms once a minute at worst.
Also holds and stops the sweep timer. AutoDenylistFilter.Start registered a
recurring tokenless Timer.DelayCall while Stop() was empty, so a Stop/Start
cycle left the old sweep running and added another -- the same bug just
fixed in BlocklistFilter.
The new test fails without the throttle (5 sweeps instead of 1), verified
by disabling it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6dcb59e00b
commit
a5c785c4a0
3 changed files with 61 additions and 6 deletions
|
|
@ -20,8 +20,8 @@ using Xunit;
|
|||
|
||||
namespace Server.Tests.Network.AutoDenylists;
|
||||
|
||||
// Static store, so every test resets it first. Addresses come from TEST-NET-2 (198.51.100.0/24).
|
||||
// Sequential: the cap tests reach Sweep, which rents from STArrayPool, which is not thread-safe.
|
||||
// Static store, so every test resets it first and none may run alongside another.
|
||||
// Addresses come from TEST-NET-2 (198.51.100.0/24).
|
||||
[Collection("Sequential UOContent Tests")]
|
||||
public class AutoDenylistTests
|
||||
{
|
||||
|
|
@ -104,6 +104,30 @@ public class AutoDenylistTests
|
|||
Assert.False(AutoDenylist.IsDenied(IPAddress.Parse("198.51.100.109"), Now));
|
||||
}
|
||||
|
||||
// A sweep is O(maxEntries). Without the throttle a distinct-source flood re-scans the whole
|
||||
// dictionary for every address it rejects, which is the case the cap exists to make cheap.
|
||||
[Fact]
|
||||
public void Cap_does_not_re_sweep_for_every_rejected_address()
|
||||
{
|
||||
Reset(maxEntries: 2);
|
||||
|
||||
AutoDenylist.Hold(IPAddress.Parse("198.51.100.40"), BanReasons.InvalidSeed, Now);
|
||||
AutoDenylist.Hold(IPAddress.Parse("198.51.100.41"), BanReasons.InvalidSeed, Now);
|
||||
Assert.Equal(0, AutoDenylist.SweepCount);
|
||||
|
||||
// At the cap with nothing lapsed: the first rejection sweeps, the rest in the window do not.
|
||||
for (var i = 0; i < 5; i++)
|
||||
{
|
||||
Assert.False(AutoDenylist.Hold(IPAddress.Parse($"198.51.100.{50 + i}"), BanReasons.InvalidSeed, Now));
|
||||
}
|
||||
|
||||
Assert.Equal(1, AutoDenylist.SweepCount);
|
||||
|
||||
// Past the window it may try again, since entries can have lapsed by then.
|
||||
Assert.False(AutoDenylist.Hold(IPAddress.Parse("198.51.100.60"), BanReasons.InvalidSeed, Now + 1000));
|
||||
Assert.Equal(2, AutoDenylist.SweepCount);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Reaching_the_cap_reclaims_lapsed_entries_first()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -39,13 +39,22 @@ public static class AutoDenylist
|
|||
// Address (normalized v6 bits) -> Core.TickCount at which the hold lapses. Loop-only.
|
||||
private static readonly Dictionary<UInt128, long> _held = [];
|
||||
|
||||
// Shortest gap between cap-triggered sweeps. A sweep is O(_maxEntries) and one that just ran cannot
|
||||
// have freed more, so without this every rejected address re-scans the whole dictionary -- turning the
|
||||
// bound into a cost multiplier under exactly the flood it exists to bound.
|
||||
private const long SweepThrottleMs = 1000;
|
||||
|
||||
private static bool _enabled;
|
||||
private static long _durationMs;
|
||||
private static int _maxEntries;
|
||||
private static bool _warnedFull;
|
||||
private static long _lastSweepAt;
|
||||
|
||||
public static int Count => _held.Count;
|
||||
|
||||
// Test seam: lets the throttle be asserted rather than inferred.
|
||||
internal static int SweepCount { get; private set; }
|
||||
|
||||
public static void Configure()
|
||||
{
|
||||
AutoDenylistConfiguration.Load();
|
||||
|
|
@ -60,6 +69,9 @@ public static class AutoDenylist
|
|||
_durationMs = (long)s.Duration.TotalMilliseconds;
|
||||
_maxEntries = s.MaxEntries;
|
||||
|
||||
// Seeded from a real tick: tick counts need not start near zero. See dev-docs/tick-counts.md.
|
||||
_lastSweepAt = Core.TickCount;
|
||||
|
||||
ConnectionFilters.Register(new AutoDenylistFilter());
|
||||
BanChannel.Register(new AutoDenylistReporter());
|
||||
}
|
||||
|
|
@ -82,7 +94,10 @@ public static class AutoDenylist
|
|||
// An address already held is just extended, so no cap check is needed.
|
||||
if (!_held.ContainsKey(key) && _held.Count >= _maxEntries)
|
||||
{
|
||||
Sweep(nowTicks);
|
||||
if (nowTicks - _lastSweepAt >= SweepThrottleMs)
|
||||
{
|
||||
Sweep(nowTicks);
|
||||
}
|
||||
|
||||
if (_held.Count >= _maxEntries)
|
||||
{
|
||||
|
|
@ -128,6 +143,10 @@ public static class AutoDenylist
|
|||
|
||||
internal static void Sweep(long nowTicks)
|
||||
{
|
||||
// Stamped even when there is nothing to do, so the cap path throttles off the last attempt.
|
||||
_lastSweepAt = nowTicks;
|
||||
SweepCount++;
|
||||
|
||||
if (_held.Count == 0)
|
||||
{
|
||||
return;
|
||||
|
|
@ -157,12 +176,16 @@ public static class AutoDenylist
|
|||
_durationMs = durationMs;
|
||||
_maxEntries = maxEntries;
|
||||
_warnedFull = false;
|
||||
_lastSweepAt = 0;
|
||||
SweepCount = 0;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Accept-path gate for <see cref="AutoDenylist"/>.</summary>
|
||||
public sealed class AutoDenylistFilter : IConnectionFilter
|
||||
{
|
||||
private Timer _sweepTimer;
|
||||
|
||||
public string Name => "auto-denylist";
|
||||
|
||||
public void Register()
|
||||
|
|
@ -171,12 +194,19 @@ public sealed class AutoDenylistFilter : IConnectionFilter
|
|||
|
||||
public void Start(CancellationToken token)
|
||||
{
|
||||
// Only an optimisation: IsDenied expires on read.
|
||||
Timer.DelayCall(TimeSpan.FromMinutes(1), TimeSpan.FromMinutes(1), () => AutoDenylist.Sweep(Core.TickCount));
|
||||
// Only an optimization: IsDenied expires on read.
|
||||
_sweepTimer = Timer.DelayCall(
|
||||
TimeSpan.FromMinutes(1),
|
||||
TimeSpan.FromMinutes(1),
|
||||
() => AutoDenylist.Sweep(Core.TickCount)
|
||||
);
|
||||
}
|
||||
|
||||
public void Stop()
|
||||
{
|
||||
// Recurring, so an uncancelled sweep survives Stop and the next Start adds a second one.
|
||||
_sweepTimer?.Stop();
|
||||
_sweepTimer = null;
|
||||
}
|
||||
|
||||
public bool ShouldDeny(IPAddress address) => AutoDenylist.IsDenied(address);
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ using System.Globalization;
|
|||
using System.IO;
|
||||
using System.Net;
|
||||
using System.Text;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Server.Logging;
|
||||
using Server.Network.Bans;
|
||||
|
|
@ -259,7 +260,7 @@ public static class LoginAllowlist
|
|||
/// </summary>
|
||||
private static void OnCrashed(ServerCrashedEventArgs e)
|
||||
{
|
||||
if (System.Threading.Thread.CurrentThread == Core.Thread)
|
||||
if (Thread.CurrentThread == Core.Thread)
|
||||
{
|
||||
OnShutdown();
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue