From 412a71dfe0ee542f75a4dfe38edbdcd3f8abe5f0 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sun, 7 Jun 2026 12:36:37 -0700 Subject: [PATCH] fix(tests): single shared bootstrap; idempotent SerializationThreadWorker.Exit (#2473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Running the full `UOContent.Tests` suite, the test host **hangs ~2.5 minutes at shutdown and then crashes** (`Test host process crashed` / run aborted). The tests themselves are fine — they complete in ~1s — but the process can't exit. Captured via `--blame-hang` dump. The blocking thread: ``` System.Threading.WaitHandle.WaitOne() Server.SerializationThreadWorker.Sleep() SerializationThreadWorker.cs:54 (_stopEvent.WaitOne()) Server.SerializationThreadWorker.Exit() SerializationThreadWorker.cs:61 Server.World.ExitSerializationThreads() World.cs:429 Server.Tests.UOContentFixture..ctor() ``` ### Root cause Both collection fixtures (`UOContentFixture` and `PathfindingTestFixture`) each run the **full process-global ModernUO bootstrap**. `World.Load()` is guarded to run once per process, so the **second** fixture's `World.Load()` is a no-op and does **not** respawn the serialization workers — but `World.ExitSerializationThreads()` is **not** guarded, so the second fixture calls `Exit()` on workers whose threads have already terminated. `Exit()` → `Sleep()` → `_stopEvent.WaitOne()` then blocks forever (a dead thread never sets the event). The first collection's tests run; the second collection's fixture deadlocks in its constructor; the host eventually gets killed. This is why single-collection (filtered) runs were fine — only one fixture ever bootstraps — but the full suite hangs. It's not a parallelization race: even strictly sequential, the second fixture deadlocks. ## Fix **(a) Engine — idempotent `SerializationThreadWorker.Exit()`** A second `Exit()` is now a safe no-op instead of a permanent block. Only the owning (main) thread calls `Exit()`, so no synchronization is needed, and the single-call production shutdown path is unchanged. **(b) Tests — one shared bootstrap, strictly sequential collections** - New `TestServerBootstrap.EnsureInitialized()` runs the superset global init **exactly once per process** (lock + once-flag). - `UOContentFixture` / `PathfindingTestFixture` slim down to delegate to it and no longer tear down global state (which the single-bootstrap model owns for the host's lifetime). - `[assembly: CollectionBehavior(DisableTestParallelization = true)]` so collections never overlap. ## Result | | Before | After | |---|---|---| | Tests run (full suite) | 258 (UOContent collection deadlocked) | **418** | | Outcome | 2.5-min hang → host crash | **418 passed, clean exit** | | Wall time | killed | **~7s** | --- .../Server.Tests/Fixtures/ServerFixture.cs | 16 +-- .../Fixtures/TestServerInitializer.cs | 2 +- .../SerializationThreadWorker.cs | 7 ++ .../Fixtures/PathfindingTestFixture.cs | 90 +------------- .../Fixtures/TestServerInitializer.cs | 115 ++++++++++++++++++ .../Fixtures/UOContentFixture.cs | 62 +--------- 6 files changed, 139 insertions(+), 153 deletions(-) create mode 100644 Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs diff --git a/Projects/Server.Tests/Fixtures/ServerFixture.cs b/Projects/Server.Tests/Fixtures/ServerFixture.cs index 1059436f0..e2795848f 100644 --- a/Projects/Server.Tests/Fixtures/ServerFixture.cs +++ b/Projects/Server.Tests/Fixtures/ServerFixture.cs @@ -1,4 +1,3 @@ -using System; using Xunit; namespace Server.Tests; @@ -8,7 +7,7 @@ namespace Server.Tests; /// Configure client path via MODERNUO_CLIENT_PATH environment variable or place files at C:\Ultima Online Classic. /// [CollectionDefinition("Sequential Server Tests", DisableParallelization = true)] -public class ServerFixture : ICollectionFixture, IDisposable +public class ServerFixture : ICollectionFixture { /// /// True if TileData was successfully loaded from client files. @@ -35,13 +34,8 @@ public class ServerFixture : ICollectionFixture, IDisposable /// public static ushort SurfaceImpassableTileId => TestServerInitializer.SurfaceImpassableTileId; - public ServerFixture() - { - TestServerInitializer.Initialize(loadTileData: true); - } - - public void Dispose() - { - Timer.Init(0); - } + // Global init runs exactly once via the shared, guarded TestServerInitializer. The single + // bootstrap owns global state for the lifetime of the test host, so there is no + // per-collection teardown — matching UOContent.Tests' TestServerInitializer pattern. + public ServerFixture() => TestServerInitializer.Initialize(loadTileData: true); } diff --git a/Projects/Server.Tests/Fixtures/TestServerInitializer.cs b/Projects/Server.Tests/Fixtures/TestServerInitializer.cs index 4620ec0ef..e84c16509 100644 --- a/Projects/Server.Tests/Fixtures/TestServerInitializer.cs +++ b/Projects/Server.Tests/Fixtures/TestServerInitializer.cs @@ -10,7 +10,7 @@ namespace Server.Tests; /// Shared server initialization logic for test fixtures. /// Ensures the server is only initialized once across all test collections. /// -public static class TestServerInitializer +internal static class TestServerInitializer { private const string DefaultDataDirectory = @"C:\Ultima Online Classic"; private static bool _initialized; diff --git a/Projects/Server/Serialization/SerializationThreadWorker.cs b/Projects/Server/Serialization/SerializationThreadWorker.cs index 3089895df..bb0e67783 100644 --- a/Projects/Server/Serialization/SerializationThreadWorker.cs +++ b/Projects/Server/Serialization/SerializationThreadWorker.cs @@ -29,6 +29,7 @@ public class SerializationThreadWorker private readonly AutoResetEvent _stopEvent; // Main thread waits for the worker finish draining private bool _pause; private bool _exit; + private bool _exited; private byte[] _heap; private readonly ConcurrentQueue _entities; @@ -56,6 +57,12 @@ public class SerializationThreadWorker public void Exit() { + if (_exited) + { + return; + } + + _exited = true; _exit = true; Wake(); Sleep(); diff --git a/Projects/UOContent.Tests/Fixtures/PathfindingTestFixture.cs b/Projects/UOContent.Tests/Fixtures/PathfindingTestFixture.cs index ee761d1c0..02dd2bbb1 100644 --- a/Projects/UOContent.Tests/Fixtures/PathfindingTestFixture.cs +++ b/Projects/UOContent.Tests/Fixtures/PathfindingTestFixture.cs @@ -1,92 +1,12 @@ -using System; -using System.IO; -using System.Reflection; -using Server.Items; -using Server.Misc; -using Server.Movement; -using Server.Tests.Maps; using Xunit; namespace Server.Tests.Pathfinding; [CollectionDefinition("Sequential Pathfinding Tests", DisableParallelization = true)] -public class PathfindingTestFixture : ICollectionFixture, IDisposable +public class PathfindingTestFixture : ICollectionFixture { - public PathfindingTestFixture() - { - Core.ApplicationAssembly = Assembly.GetExecutingAssembly(); - Core.LoopContext = new EventLoopContext(); - Core.Expansion = Expansion.EJ; - - ServerConfiguration.Load(true); - ServerConfiguration.AssemblyDirectories.Add(Core.BaseDirectory); - - var clientFiles = Environment.GetEnvironmentVariable("MODERNUO_TEST_DATA_DIR") - ?? @"C:\Ultima Online Classic"; - ServerConfiguration.DataDirectories.Add(clientFiles); - - AssemblyHandler.LoadAssemblies(["Server.dll", "UOContent.dll"]); - - SkillsInfo.Configure(); - Server.Network.NetState.Configure(); - TestMapDefinitions.ConfigureTestMapDefinitions(); - - World.Configure(); - Timer.Init(0); - RaceDefinitions.Configure(); - MovementImpl.Configure(); - PathFollower.Configure(); - World.Load(); - World.ExitSerializationThreads(); - DecayScheduler.Configure(); - - // TileData's static cctor short-circuits when running under xUnit - // (see Server/TileData.cs:295). Force-load via reflection so LandTable/ItemTable - // flags are populated; without this, every tile reads as flag=None and - // MovementImpl.CheckMovement treats everything as walkable. - ForceLoadTileData(); - - VerifyTrammelTileDataLoaded(); - } - - private static void ForceLoadTileData() - { - var loadMethod = typeof(TileData).GetMethod( - "Load", - BindingFlags.Static | BindingFlags.NonPublic - ); - if (loadMethod == null) - { - throw new InvalidOperationException( - "TileData.Load not found via reflection — engine may have refactored." - ); - } - loadMethod.Invoke(null, null); - } - - private static void VerifyTrammelTileDataLoaded() - { - var trammel = Map.Maps[1]; - if (trammel == null) - { - throw new InvalidOperationException( - "Trammel (mapId=1) was not registered. Check TestMapDefinitions." - ); - } - - var tile = trammel.Tiles.GetLandTile(1500, 1600); - if (tile.ID == 0) - { - throw new InvalidOperationException( - $"Trammel tile data did not load — GetLandTile(1500,1600) returned ID 0. " + - $"Verify Distribution/Data/map1*.mul (or map1LegacyMUL.uop) is present at " + - $"{Path.Combine(Core.BaseDirectory, "Data")}." - ); - } - } - - public void Dispose() - { - Timer.Init(0); - } + // Shares the single process-wide bootstrap (TestServerInitializer, in the parent + // Server.Tests namespace). The superset bootstrap already loads the UO client tile data + // and configures movement/pathfinding, so this collection needs no extra setup. + public PathfindingTestFixture() => TestServerInitializer.Initialize(); } diff --git a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs new file mode 100644 index 000000000..a64286813 --- /dev/null +++ b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs @@ -0,0 +1,115 @@ +using System; +using System.IO; +using System.Reflection; +using System.Threading; +using Server.Items; +using Server.Misc; +using Server.Movement; +using Server.Tests.Maps; + +namespace Server.Tests; + +/// +/// Single, process-wide ModernUO bootstrap for the UOContent test host. Mirrors Server.Tests' +/// TestServerInitializer in name and shape; kept as a separate (non-shared) copy because this +/// one loads the UOContent assembly and configures the UOContent-specific systems. Both types +/// are internal so the shared name stays scoped to each assembly. +/// +/// ModernUO bootstraps its global singletons (Core, ServerConfiguration, AssemblyHandler, +/// NetState/io-ring, World, Timer, the serialization workers, and TileData) exactly once per +/// process. is guarded to run once, and +/// must run once against the live workers. Each +/// xUnit collection gets its own fixture instance, so this guard makes the bootstrap run a +/// single time regardless of how many collection fixtures are constructed. The two stateful +/// collections use [CollectionDefinition(DisableParallelization = true)] so they never +/// overlap; pure tests still run in parallel. +/// +internal static class TestServerInitializer +{ + private static bool _initialized; + private static readonly Lock _lock = new(); + + public static void Initialize() + { + lock (_lock) + { + if (_initialized) + { + return; + } + + Core.ApplicationAssembly = Assembly.GetExecutingAssembly(); + Core.LoopContext = new EventLoopContext(); + Core.Expansion = Expansion.EJ; + + ServerConfiguration.Load(true); + ServerConfiguration.AssemblyDirectories.Add(Core.BaseDirectory); + + // Required for the pathfinding tests (real .mul tile data). Harmless for the rest. + var clientFiles = Environment.GetEnvironmentVariable("MODERNUO_TEST_DATA_DIR") + ?? @"C:\Ultima Online Classic"; + ServerConfiguration.DataDirectories.Add(clientFiles); + + AssemblyHandler.LoadAssemblies(["Server.dll", "UOContent.dll"]); + + SkillsInfo.Configure(); + Server.Network.NetState.Configure(); + TestMapDefinitions.ConfigureTestMapDefinitions(); + + World.Configure(); + Timer.Init(0); + RaceDefinitions.Configure(); + MovementImpl.Configure(); + PathFollower.Configure(); + World.Load(); + World.ExitSerializationThreads(); + DecayScheduler.Configure(); + + // TileData's static cctor short-circuits when running under xUnit + // (see Server/TileData.cs:295). Force-load via reflection so LandTable/ItemTable + // flags are populated; without this, every tile reads as flag=None and + // MovementImpl.CheckMovement treats everything as walkable. + ForceLoadTileData(); + + VerifyTrammelTileDataLoaded(); + + _initialized = true; + } + } + + private static void ForceLoadTileData() + { + var loadMethod = typeof(TileData).GetMethod( + "Load", + BindingFlags.Static | BindingFlags.NonPublic + ); + if (loadMethod == null) + { + throw new InvalidOperationException( + "TileData.Load not found via reflection — engine may have refactored." + ); + } + loadMethod.Invoke(null, null); + } + + private static void VerifyTrammelTileDataLoaded() + { + var trammel = Map.Maps[1]; + if (trammel == null) + { + throw new InvalidOperationException( + "Trammel (mapId=1) was not registered. Check TestMapDefinitions." + ); + } + + var tile = trammel.Tiles.GetLandTile(1500, 1600); + if (tile.ID == 0) + { + throw new InvalidOperationException( + $"Trammel tile data did not load — GetLandTile(1500,1600) returned ID 0. " + + $"Verify Distribution/Data/map1*.mul (or map1LegacyMUL.uop) is present at " + + $"{Path.Combine(Core.BaseDirectory, "Data")}." + ); + } + } +} diff --git a/Projects/UOContent.Tests/Fixtures/UOContentFixture.cs b/Projects/UOContent.Tests/Fixtures/UOContentFixture.cs index 4a1905922..d934fda57 100644 --- a/Projects/UOContent.Tests/Fixtures/UOContentFixture.cs +++ b/Projects/UOContent.Tests/Fixtures/UOContentFixture.cs @@ -1,63 +1,13 @@ -using System; -using System.Reflection; -using Server.Items; -using Server.Misc; -using Server.Tests.Maps; using Xunit; namespace Server.Tests; [CollectionDefinition("Sequential UOContent Tests", DisableParallelization = true)] -public class UOContentFixture : ICollectionFixture, IDisposable +public class UOContentFixture : ICollectionFixture { - public UOContentFixture() - { - Core.ApplicationAssembly = Assembly.GetExecutingAssembly(); - Core.LoopContext = new EventLoopContext(); - Core.Expansion = Expansion.EJ; - - // Load Configurations - ServerConfiguration.Load(true); - - // Load UOContent.dll into the type resolver - ServerConfiguration.AssemblyDirectories.Add(Core.BaseDirectory); - AssemblyHandler.LoadAssemblies(["Server.dll", "UOContent.dll"]); - - // Load Skills - SkillsInfo.Configure(); - - // Configure networking (initializes RingSocketManager for tests) - Server.Network.NetState.Configure(); - - // Configure / Initialize - TestMapDefinitions.ConfigureTestMapDefinitions(); - - // Configure the world - World.Configure(); - - Timer.Init(0); - - // Configure Races - RaceDefinitions.Configure(); - - // Load the world - World.Load(); - - World.ExitSerializationThreads(); - - DecayScheduler.Configure(); - } - - private static int _counter; - - public void Dispose() - { - _counter++; - - if (_counter > 1) - { - throw new Exception("NO!"); - } - Timer.Init(0); - } + // All process-global initialization lives in TestServerInitializer and runs exactly once, + // shared across every collection fixture. Tearing down global state here is intentionally + // omitted: the world/serialization workers are initialized once and reused for the whole + // test host, so there is nothing per-collection to dispose. + public UOContentFixture() => TestServerInitializer.Initialize(); }