From 583177b5057e48b2e2897bd60c332e6eeacd58da Mon Sep 17 00:00:00 2001 From: penspanic Date: Tue, 14 Jul 2026 09:17:13 +0900 Subject: [PATCH] Fix broken critical section in static World create/destroy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit World.Create and World.Dispose locked the static Worlds array itself, but Create replaces that array on resize. After a resize, a creator blocked on the old array's monitor and a newly arriving creator locking the new array enter the "critical section" concurrently: two worlds can be handed the same id (cross-wiring EntityInfo lookups into AccessViolation deep in Chunk), and a slot write can land in the stale array so the live Worlds array holds null for a created world (NullReferenceException in EntityExtensions/World.Get right after Create). Readers also indexed Worlds with no publication barrier, so on weakly-ordered CPUs (ARM64) a resized array could become visible before its copied contents. Guard create/destroy with a dedicated lock object, fill the resized copy before publishing it with a volatile write, and write slots volatile. Readers stay lock-free: element loads carry an address dependency on the array reference, so release-publication is sufficient. Adds WorldConcurrencyTest covering concurrent create (forcing resizes) and create/use/destroy churn — both fail against the previous locking in 3/3 runs. WorldRecycle now drains the recycled-id queue first so it no longer depends on the queue being empty when the fixture runs. Co-Authored-By: Claude Fable 5 --- src/Arch.Tests/WorldConcurrencyTest.cs | 135 +++++++++++++++++++++++++ src/Arch.Tests/WorldTest.cs | 28 ++++- src/Arch/Core/World.cs | 45 +++++++-- 3 files changed, 193 insertions(+), 15 deletions(-) create mode 100644 src/Arch.Tests/WorldConcurrencyTest.cs diff --git a/src/Arch.Tests/WorldConcurrencyTest.cs b/src/Arch.Tests/WorldConcurrencyTest.cs new file mode 100644 index 00000000..a0559ed8 --- /dev/null +++ b/src/Arch.Tests/WorldConcurrencyTest.cs @@ -0,0 +1,135 @@ +// The static World registry (World.Worlds / World.Create bookkeeping) does not exist +// under PURE_ECS, and neither do the Entity extension methods used below. +#if !PURE_ECS +using Arch.Core; +using Arch.Core.Extensions; +using static NUnit.Framework.Assert; + +namespace Arch.Tests; + +/// +/// The class +/// tests concurrent / against the +/// static storage. +/// +/// Historically World.Create locked the array itself and +/// replaced that array on resize, so after a resize concurrent creators locked different +/// objects and raced: duplicate world ids, lost slot writes (a created world resolving to +/// null through World.Worlds[entity.WorldId]) and cross-wired entity storage. +/// +[TestFixture] +public sealed class WorldConcurrencyTest +{ + /// + /// Concurrently creates worlds (forcing several resizes), + /// uses each created world immediately and checks id uniqueness. + /// + [Test] + public void ConcurrentWorldCreateProducesUniqueUsableWorlds() + { + const int threads = 8; + const int worldsPerThread = 64; + + var created = new World[threads * worldsPerThread]; + using var barrier = new Barrier(threads); + + RunOnThreads(threads, threadIndex => + { + barrier.SignalAndWait(); + for (var i = 0; i < worldsPerThread; i++) + { + var world = World.Create(); + created[(threadIndex * worldsPerThread) + i] = world; + + // Use the world through the static lookup right away — this is the read path + // (EntityExtensions/generated accessors) that observed null slots pre-fix. + var entity = world.Create(new Transform { X = threadIndex, Y = i }); + That(entity.IsAlive(), Is.True); + That(entity.Get().X, Is.EqualTo(threadIndex)); + } + }); + + try + { + var ids = new HashSet(); + foreach (var world in created) + { + That(world, Is.Not.Null); + That(ids.Add(world.Id), Is.True, $"Duplicate world id {world.Id} handed out concurrently."); + That(World.Worlds[world.Id], Is.SameAs(world)); + } + } + finally + { + foreach (var world in created) + { + if (world != null) + { + World.Destroy(world); + } + } + } + } + + /// + /// Churns concurrent create → use → destroy cycles so id recycling, slot writes and + /// resizes interleave across threads. + /// + [Test] + public void ConcurrentWorldCreateDestroyChurnDoesNotCorruptStaticStorage() + { + const int threads = 8; + const int rounds = 200; + + using var barrier = new Barrier(threads); + + RunOnThreads(threads, threadIndex => + { + barrier.SignalAndWait(); + for (var round = 0; round < rounds; round++) + { + var world = World.Create(); + var entity = world.Create(new Transform { X = round, Y = threadIndex }); + That(entity.Get().Y, Is.EqualTo(threadIndex)); + That(World.Worlds[world.Id], Is.SameAs(world)); + World.Destroy(world); + } + }); + } + + private static void RunOnThreads(int threadCount, Action body) + { + var failures = new List(); + var workers = new Thread[threadCount]; + for (var t = 0; t < threadCount; t++) + { + var threadIndex = t; + workers[t] = new Thread(() => + { + try + { + body(threadIndex); + } + catch (Exception exception) + { + lock (failures) + { + failures.Add(exception); + } + } + }); + workers[t].Start(); + } + + foreach (var worker in workers) + { + worker.Join(); + } + + if (failures.Count > 0) + { + throw new AggregateException(failures); + } + } +} +#endif diff --git a/src/Arch.Tests/WorldTest.cs b/src/Arch.Tests/WorldTest.cs index d6d47b9f..d43e3ab5 100644 --- a/src/Arch.Tests/WorldTest.cs +++ b/src/Arch.Tests/WorldTest.cs @@ -48,11 +48,31 @@ public void Teardown() [Test] public void WorldRecycle() { - var firstWorld = World.Create(); - World.Destroy(firstWorld); + // Recycled ids are handed out FIFO, so drain the queue left behind by earlier + // world-churning tests to make the reuse check below deterministic. The queue can + // never hold more ids than the id space allocated so far, which Worlds.Length bounds. + var drained = new World[World.Worlds.Length]; + for (var index = 0; index < drained.Length; index++) + { + drained[index] = World.Create(); + } - var secondWorld = World.Create(); - That(secondWorld.Id, Is.EqualTo(firstWorld.Id)); + try + { + var firstWorld = World.Create(); + World.Destroy(firstWorld); + + var secondWorld = World.Create(); + That(secondWorld.Id, Is.EqualTo(firstWorld.Id)); + World.Destroy(secondWorld); + } + finally + { + foreach (var world in drained) + { + World.Destroy(world); + } + } } /// diff --git a/src/Arch/Core/World.cs b/src/Arch/Core/World.cs index d544b1b2..82c6a5ab 100644 --- a/src/Arch/Core/World.cs +++ b/src/Arch/Core/World.cs @@ -72,7 +72,22 @@ public partial class World /// A list of all existing . /// Should not be modified by the user. /// - public static World[] Worlds { get; private set; } = new World[4]; + public static World[] Worlds + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => _worlds; + } + + private static World[] _worlds = new World[4]; + + /// + /// Guards , and world id assignment. + /// A dedicated lock object: locking the array itself is unsound because the array + /// reference is replaced on resize, after which concurrent creators lock different + /// objects and race — duplicate world ids and lost slot writes (NREs in + /// , AccessViolation in ). + /// + private static readonly object WorldsLock = new(); /// /// Stores recycled IDs. @@ -106,23 +121,31 @@ public static World Create(int chunkSizeInBytes = 16_384, int minimumAmountOfEnt #if PURE_ECS return new World(-1, chunkSizeInBytes, minimumAmountOfEntitiesPerChunk, archetypeCapacity, entityCapacity); #else - lock (Worlds) + lock (WorldsLock) { var recycle = RecycledWorldIds.TryDequeue(out var id); var recycledId = recycle ? id : WorldSize; var world = new World(recycledId, chunkSizeInBytes, minimumAmountOfEntitiesPerChunk, archetypeCapacity, entityCapacity); - // If you need to ensure a higher capacity, you can manually check and increase it - if (recycledId >= Worlds.Length) + var worlds = _worlds; + if (recycledId >= worlds.Length) + { + // Fill the slot in the copy before publishing the new array so readers + // (EntityExtensions and generated accessors index Worlds without a lock, + // and Entity handles carry an address dependency on the array reference) + // never observe a published array whose contents are not yet visible on + // weakly-ordered CPUs (ARM64). + var resized = new World[worlds.Length * 2]; + Array.Copy(worlds, resized, worlds.Length); + resized[recycledId] = world; + Volatile.Write(ref _worlds, resized); + } + else { - var newCapacity = Worlds.Length * 2; - var worlds = Worlds; - Array.Resize(ref worlds, newCapacity); - Worlds = worlds; + Volatile.Write(ref worlds[recycledId], world); } - Worlds[recycledId] = world; Interlocked.Increment(ref worldSizeUnsafe); return world; } @@ -533,9 +556,9 @@ protected virtual void Dispose(bool disposing) _isDisposed = true; var world = this; #if !PURE_ECS - lock (Worlds) + lock (WorldsLock) { - Worlds[world.Id] = null!; + Volatile.Write(ref _worlds[world.Id], null!); RecycledWorldIds.Enqueue(world.Id); Interlocked.Decrement(ref worldSizeUnsafe); }