fix(A.5 T10-T12): Start() race + null mesh test + real mesh stub
Code review on T10-T12 bundle (commits 0cf86bb/00bb030/0405947 + audit
fix 76e1a64) found 3 Important issues:
1. LandblockStreamer.Start() had an idempotency race — the XML doc
claimed thread-safety but the implementation checked _worker != null
before assigning, allowing two callers to both pass the check and
spawn duplicate worker threads. Fixed via Interlocked.CompareExchange.
2. No test verified the worker emits Failed when buildMeshOrNull returns
null. Added Load_WhenBuildMeshReturnsNull_ReportsFailed.
3. StreamingControllerTests.cs:81 used MeshData: default! when
constructing a Loaded result. If a future test flows MeshData
through the apply callback, the null reference would NRE rather
than producing a meaningful assertion failure. Replaced with a real
empty LandblockMeshData instance.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
76e1a64d78
commit
774a7070a8
3 changed files with 52 additions and 6 deletions
|
|
@ -75,20 +75,27 @@ public sealed class LandblockStreamer : IDisposable
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Activate the dedicated background worker thread. Idempotent: calling
|
/// Activate the dedicated background worker thread. Idempotent and
|
||||||
/// <see cref="Start"/> more than once has no effect.
|
/// thread-safe: concurrent callers will only spawn one worker; subsequent
|
||||||
|
/// calls are no-ops. Atomic via <see cref="Interlocked.CompareExchange{T}(ref T, T, T)"/>.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public void Start()
|
public void Start()
|
||||||
{
|
{
|
||||||
if (System.Threading.Volatile.Read(ref _disposed) != 0)
|
if (System.Threading.Volatile.Read(ref _disposed) != 0)
|
||||||
throw new ObjectDisposedException(nameof(LandblockStreamer));
|
throw new ObjectDisposedException(nameof(LandblockStreamer));
|
||||||
if (_worker != null) return;
|
|
||||||
_worker = new Thread(WorkerLoop)
|
// A.5 T10-T12 follow-up: atomically install the worker so concurrent
|
||||||
|
// Start() callers don't both pass the null check and spawn duplicate
|
||||||
|
// threads. Construct the candidate; CAS it into _worker; if we lost
|
||||||
|
// the race, the candidate goes unstarted and is GCed.
|
||||||
|
var candidate = new Thread(WorkerLoop)
|
||||||
{
|
{
|
||||||
IsBackground = true,
|
IsBackground = true,
|
||||||
Name = "acdream.streaming.worker",
|
Name = "acdream.streaming.worker",
|
||||||
};
|
};
|
||||||
_worker.Start();
|
if (Interlocked.CompareExchange(ref _worker, candidate, null) == null)
|
||||||
|
candidate.Start();
|
||||||
|
// else: another caller won the race; their thread is running.
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|
|
||||||
|
|
@ -66,6 +66,39 @@ public class LandblockStreamerTests
|
||||||
Assert.IsType<LandblockStreamResult.Failed>(result);
|
Assert.IsType<LandblockStreamResult.Failed>(result);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Load_WhenBuildMeshReturnsNull_ReportsFailed()
|
||||||
|
{
|
||||||
|
// Phase A.5 T10-T12 follow-up: the mesh-build factory may return
|
||||||
|
// null (e.g., LandBlock dat missing or corrupt). The worker must
|
||||||
|
// emit Failed in that case instead of constructing Loaded with a
|
||||||
|
// null MeshData (which would NRE downstream).
|
||||||
|
var stubLandblock = new LoadedLandblock(
|
||||||
|
0xABCDFFFEu,
|
||||||
|
new LandBlock(),
|
||||||
|
System.Array.Empty<WorldEntity>());
|
||||||
|
|
||||||
|
using var streamer = new LandblockStreamer(
|
||||||
|
loadLandblock: _ => stubLandblock,
|
||||||
|
buildMeshOrNull: (_, _) => null); // mesh-build returns null
|
||||||
|
|
||||||
|
streamer.Start();
|
||||||
|
streamer.EnqueueLoad(0xABCDFFFEu);
|
||||||
|
|
||||||
|
LandblockStreamResult? result = null;
|
||||||
|
for (int i = 0; i < SpinMaxIterations && result is null; i++)
|
||||||
|
{
|
||||||
|
var drained = streamer.DrainCompletions(LandblockStreamer.DefaultDrainBatchSize);
|
||||||
|
if (drained.Count > 0) result = drained[0];
|
||||||
|
else await Task.Delay(SpinStepMs);
|
||||||
|
}
|
||||||
|
|
||||||
|
Assert.NotNull(result);
|
||||||
|
var failed = Assert.IsType<LandblockStreamResult.Failed>(result);
|
||||||
|
Assert.Equal(0xABCDFFFEu, failed.LandblockId);
|
||||||
|
Assert.Contains("mesh", failed.Error, System.StringComparison.OrdinalIgnoreCase);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task Load_WhenLoaderThrows_ReportsFailedWithMessage()
|
public async Task Load_WhenLoaderThrows_ReportsFailedWithMessage()
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -78,7 +78,13 @@ public class StreamingControllerTests
|
||||||
// Entities (positional record). Adjust if the first positional arg
|
// Entities (positional record). Adjust if the first positional arg
|
||||||
// name differs.
|
// name differs.
|
||||||
var lb = new LoadedLandblock(0x32320FFEu, new LandBlock(), System.Array.Empty<WorldEntity>());
|
var lb = new LoadedLandblock(0x32320FFEu, new LandBlock(), System.Array.Empty<WorldEntity>());
|
||||||
fake.Pending.Enqueue(new LandblockStreamResult.Loaded(0x32320FFEu, LandblockStreamTier.Near, lb, MeshData: default!));
|
// A.5 T10-T12 follow-up: use a real empty mesh instance instead of
|
||||||
|
// default! so any future test that flows MeshData through the apply
|
||||||
|
// callback gets a non-null reference to inspect rather than an NRE.
|
||||||
|
var stubMesh = new AcDream.Core.Terrain.LandblockMeshData(
|
||||||
|
System.Array.Empty<AcDream.Core.Terrain.TerrainVertex>(),
|
||||||
|
System.Array.Empty<uint>());
|
||||||
|
fake.Pending.Enqueue(new LandblockStreamResult.Loaded(0x32320FFEu, LandblockStreamTier.Near, lb, stubMesh));
|
||||||
|
|
||||||
controller.Tick(50, 50);
|
controller.Tick(50, 50);
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue