From 24cd9c17305911d67c225e6c14d2397a1d71bd1d Mon Sep 17 00:00:00 2001 From: Kevin Ferrare Date: Mon, 22 Jun 2026 19:33:46 +0200 Subject: [PATCH] fix: Fix headless gui and clock keeping references to emulator and making tests very memory hungry. Added a test to verify this doesnt happen anymore. --- .../Spice86.MicroBenchmarkTemplate.csproj | 1 + src/Spice86/Spice86DependencyInjection.cs | 12 +- .../ViewModels/Services/HeadlessGui.cs | 6 + .../SingleStepTests/SingleStepTest.cs | 2 +- .../HttpApiGeneratedClientIntegrationTests.cs | 7 +- .../Spice86.Tests/Http/HttpApiServerTests.cs | 59 ++++----- tests/Spice86.Tests/MachineLeakGuard.cs | 24 ++++ tests/Spice86.Tests/MachineLeakTracker.cs | 39 ++++++ tests/Spice86.Tests/Spice86.Tests.csproj | 4 +- tests/Spice86.Tests/Spice86Creator.cs | 1 + .../Spice86.Tests/Video/RendererTestsBase.cs | 122 +++++++++--------- .../Video/VgaRenderer256ColorTestsBase.cs | 32 ++--- 12 files changed, 194 insertions(+), 115 deletions(-) create mode 100644 tests/Spice86.Tests/MachineLeakGuard.cs create mode 100644 tests/Spice86.Tests/MachineLeakTracker.cs diff --git a/src/Spice86.MicroBenchmarkTemplate/Spice86.MicroBenchmarkTemplate.csproj b/src/Spice86.MicroBenchmarkTemplate/Spice86.MicroBenchmarkTemplate.csproj index bfaf476a6a..2f552d23bf 100644 --- a/src/Spice86.MicroBenchmarkTemplate/Spice86.MicroBenchmarkTemplate.csproj +++ b/src/Spice86.MicroBenchmarkTemplate/Spice86.MicroBenchmarkTemplate.csproj @@ -3,6 +3,7 @@ False Exe + Spice86.MicroBenchmarkTemplate.Program net10.0 enable enable diff --git a/src/Spice86/Spice86DependencyInjection.cs b/src/Spice86/Spice86DependencyInjection.cs index 594f27849d..71fee4cce8 100644 --- a/src/Spice86/Spice86DependencyInjection.cs +++ b/src/Spice86/Spice86DependencyInjection.cs @@ -216,12 +216,16 @@ internal Spice86DependencyInjection(Configuration configuration, MainWindow? mai } DateTimeOffset clockStartTime = configuration.ClockStartTime ?? DateTimeOffset.UtcNow; - _emulatedClock = configuration.InstructionTimeScale != null + IEmulatedClock emulatedClock = configuration.InstructionTimeScale != null ? new CyclesClock(state, configuration.InstructionTimeScale.Value, configuration.ClockJitterSeed, clockStartTime) : new EmulatedClock(configuration.ClockJitterSeed, clockStartTime); - // Register clock and limiter to pause/resume events - pauseHandler.Pausing += () => _emulatedClock.OnPause(); - pauseHandler.Resumed += () => _emulatedClock.OnResume(); + _emulatedClock = emulatedClock; + // Register clock and limiter to pause/resume events. Capture the clock through a local rather than the + // field so the event delegates do not close over this Spice86DependencyInjection instance: the pause + // handler can be rooted independently (e.g. by the MCP host service graph), and a closure over 'this' + // would keep the whole Machine graph alive after disposal. + pauseHandler.Pausing += () => emulatedClock.OnPause(); + pauseHandler.Resumed += () => emulatedClock.OnResume(); DeviceScheduler emulationLoopScheduler = new(_emulatedClock, loggerService, "Emulation loop"); FloppyDiskTimingService floppyDiskTimingService = new(state, _emulatedClock, emulationLoopScheduler, FloppyDiskSpeed.Maximum); diff --git a/src/Spice86/ViewModels/Services/HeadlessGui.cs b/src/Spice86/ViewModels/Services/HeadlessGui.cs index fc257b6bde..fbc315986b 100644 --- a/src/Spice86/ViewModels/Services/HeadlessGui.cs +++ b/src/Spice86/ViewModels/Services/HeadlessGui.cs @@ -130,6 +130,12 @@ private void Dispose(bool disposing) { return; } + // These are process-lifetime static events; without unsubscribing they keep every + // HeadlessGui instance (and the whole Machine graph it references) alive for the + // lifetime of the process, which leaks an entire emulator per instance. + AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; + Console.CancelKeyPress -= OnProcessExit; + // Stop the timer to prevent any new callbacks _drawTimer?.Dispose(); diff --git a/tests/Spice86.Tests/CpuTests/SingleStepTests/SingleStepTest.cs b/tests/Spice86.Tests/CpuTests/SingleStepTests/SingleStepTest.cs index 63d15c2dbe..881cb01bc2 100644 --- a/tests/Spice86.Tests/CpuTests/SingleStepTests/SingleStepTest.cs +++ b/tests/Spice86.Tests/CpuTests/SingleStepTests/SingleStepTest.cs @@ -152,7 +152,7 @@ private void RunAllTests(ZipArchive archive, ISet revocationList, ISet { private const string KiotaToolVersion = "1.30.0"; private static readonly TimeSpan CommandTimeout = TimeSpan.FromMinutes(3); private readonly HttpApiServerFixture _fixture; @@ -25,6 +24,10 @@ public HttpApiGeneratedClientIntegrationTests(HttpApiServerFixture fixture) { [InlineData("yaml", "/openapi/v1.yaml")] public async Task KiotaGeneratedDotNetClient_CanBeGeneratedBuiltAndExecuted(string extension, string openApiPath) { // Arrange + _fixture.Memory[0x40] = 0x12; + _fixture.Memory[0x41] = 0x34; + _fixture.Memory[0x42] = 0x56; + _fixture.Memory[0x43] = 0x78; _fixture.PauseHandler.IsPaused.Returns(false); _fixture.PauseHandler.ClearReceivedCalls(); TestWorkspace workspace = CreateTestWorkspace(extension); diff --git a/tests/Spice86.Tests/Http/HttpApiServerTests.cs b/tests/Spice86.Tests/Http/HttpApiServerTests.cs index a4c5259e22..2c0384e001 100644 --- a/tests/Spice86.Tests/Http/HttpApiServerTests.cs +++ b/tests/Spice86.Tests/Http/HttpApiServerTests.cs @@ -30,11 +30,12 @@ private void SeedMemory() { [Fact] public async Task GetStatus_ReturnsMachineState() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/status"); + _fixture.PauseHandler.IsPaused.Returns(false); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/status", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); - HttpApiStatusResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiStatusResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.IsPaused.Should().BeFalse(); payload.IsCpuRunning.Should().BeTrue(); @@ -47,11 +48,11 @@ public async Task GetStatus_ReturnsMachineState() { [Fact] public async Task GetByte_ReturnsByteAtAddress() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/byte"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/byte", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); - HttpApiMemoryByteResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiMemoryByteResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Address.Should().Be(64); payload.Value.Should().Be(0x12); @@ -63,7 +64,7 @@ public async Task GetByte_WhenNotPaused_PausesThenUnpauses() { _fixture.PauseHandler.ClearReceivedCalls(); _fixture.PauseHandler.IsPaused.Returns(false); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/byte"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/byte", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); _fixture.PauseHandler.Received(1).RequestPause(Arg.Any()); @@ -76,7 +77,7 @@ public async Task GetByte_WhenAlreadyPaused_DoesNotUnpause() { _fixture.PauseHandler.ClearReceivedCalls(); _fixture.PauseHandler.IsPaused.Returns(true); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/byte"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/byte", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); _fixture.PauseHandler.DidNotReceive().RequestPause(Arg.Any()); @@ -90,15 +91,15 @@ public async Task PutByte_WritesAndReadsBackValue() { Value = 0xAB }; - HttpResponseMessage putResponse = await _fixture.HttpClient.PutAsJsonAsync("/api/memory/64/byte", request); + HttpResponseMessage putResponse = await _fixture.HttpClient.PutAsJsonAsync("/api/memory/64/byte", request, TestContext.Current.CancellationToken); putResponse.StatusCode.Should().Be(HttpStatusCode.OK); - HttpApiMemoryByteResponse putPayload = await putResponse.Content.ReadFromJsonAsync() + HttpApiMemoryByteResponse putPayload = await putResponse.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null putPayload"); putPayload.Value.Should().Be(0xAB); - HttpResponseMessage getResponse = await _fixture.HttpClient.GetAsync("/api/memory/64/byte"); - HttpApiMemoryByteResponse getPayload = await getResponse.Content.ReadFromJsonAsync() + HttpResponseMessage getResponse = await _fixture.HttpClient.GetAsync("/api/memory/64/byte", TestContext.Current.CancellationToken); + HttpApiMemoryByteResponse getPayload = await getResponse.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null getPayload"); getPayload.Value.Should().Be(0xAB); _fixture.Memory[0x40].Should().Be(0xAB); @@ -113,7 +114,7 @@ public async Task PutByte_WhenNotPaused_PausesThenUnpauses() { Value = 0xCD }; - HttpResponseMessage response = await _fixture.HttpClient.PutAsJsonAsync("/api/memory/64/byte", request); + HttpResponseMessage response = await _fixture.HttpClient.PutAsJsonAsync("/api/memory/64/byte", request, TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); _fixture.PauseHandler.Received(1).RequestPause(Arg.Any()); @@ -123,11 +124,11 @@ public async Task PutByte_WhenNotPaused_PausesThenUnpauses() { [Fact] public async Task GetRange_ReturnsRequestedRange() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/range/4"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/range/4", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); - HttpApiMemoryRangeResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiMemoryRangeResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Address.Should().Be(64); payload.Length.Should().Be(4); @@ -138,7 +139,7 @@ public async Task GetRange_ReturnsRequestedRange() { public async Task PauseEndpoint_PausesEmulator() { _fixture.PauseHandler.ClearReceivedCalls(); - HttpResponseMessage response = await _fixture.HttpClient.PostAsync("/api/status/pause", content: null); + HttpResponseMessage response = await _fixture.HttpClient.PostAsync("/api/status/pause", content: null, TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); _fixture.PauseHandler.Received(1).RequestPause(Arg.Any()); @@ -148,7 +149,7 @@ public async Task PauseEndpoint_PausesEmulator() { public async Task UnpauseEndpoint_ResumesEmulator() { _fixture.PauseHandler.ClearReceivedCalls(); - HttpResponseMessage response = await _fixture.HttpClient.PostAsync("/api/status/unpause", content: null); + HttpResponseMessage response = await _fixture.HttpClient.PostAsync("/api/status/unpause", content: null, TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); _fixture.PauseHandler.Received(1).Resume(); @@ -161,11 +162,11 @@ public async Task GetRange_TruncatesAtMemoryEnd() { _fixture.Memory[(uint)lastAddress] = 0x9A; _fixture.Memory[(uint)(lastAddress + 1)] = 0xBC; - HttpResponseMessage response = await _fixture.HttpClient.GetAsync($"/api/memory/{lastAddress}/range/16"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync($"/api/memory/{lastAddress}/range/16", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.OK); - HttpApiMemoryRangeResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiMemoryRangeResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Length.Should().Be(2); payload.Values.Should().Equal([0x9A, 0xBC]); @@ -174,11 +175,11 @@ public async Task GetRange_TruncatesAtMemoryEnd() { [Fact] public async Task GetByte_WithNegativeAddress_ReturnsBadRequest() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/-1/byte"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/-1/byte", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.BadRequest); - HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Message.Should().Contain("between 0 and 4294967295"); } @@ -186,11 +187,11 @@ public async Task GetByte_WithNegativeAddress_ReturnsBadRequest() { [Fact] public async Task GetByte_WithAddressTooLarge_ReturnsBadRequest() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/4294967296/byte"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/4294967296/byte", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.BadRequest); - HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Message.Should().Contain("between 0 and 4294967295"); } @@ -198,11 +199,11 @@ public async Task GetByte_WithAddressTooLarge_ReturnsBadRequest() { [Fact] public async Task GetByte_WithOutOfRangeAddress_ReturnsNotFound() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync($"/api/memory/{_fixture.Memory.Length}/byte"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync($"/api/memory/{_fixture.Memory.Length}/byte", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.NotFound); - HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Message.Should().Be("address is outside of memory range"); } @@ -211,11 +212,11 @@ public async Task GetByte_WithOutOfRangeAddress_ReturnsNotFound() { public async Task GetRange_WithExcessiveLength_ReturnsBadRequest() { SeedMemory(); int excessiveLength = HttpApiEndpoint.MaxRangeLength + 1; - HttpResponseMessage response = await _fixture.HttpClient.GetAsync($"/api/memory/64/range/{excessiveLength}"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync($"/api/memory/64/range/{excessiveLength}", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.BadRequest); - HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Message.Should().Contain($"{HttpApiEndpoint.MaxRangeLength}"); } @@ -223,11 +224,11 @@ public async Task GetRange_WithExcessiveLength_ReturnsBadRequest() { [Fact] public async Task GetRange_WithInvalidLength_ReturnsBadRequest() { SeedMemory(); - HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/range/0"); + HttpResponseMessage response = await _fixture.HttpClient.GetAsync("/api/memory/64/range/0", TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.BadRequest); - HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Message.Should().Be("length must be greater than 0"); } @@ -239,11 +240,11 @@ public async Task PutByte_WithOutOfRangeAddress_ReturnsNotFound() { Value = 0xEF }; - HttpResponseMessage response = await _fixture.HttpClient.PutAsJsonAsync($"/api/memory/{_fixture.Memory.Length}/byte", request); + HttpResponseMessage response = await _fixture.HttpClient.PutAsJsonAsync($"/api/memory/{_fixture.Memory.Length}/byte", request, TestContext.Current.CancellationToken); response.StatusCode.Should().Be(HttpStatusCode.NotFound); - HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync() + HttpApiErrorResponse payload = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken) ?? throw new InvalidOperationException("Expected non-null payload"); payload.Message.Should().Be("address is outside of memory range"); } diff --git a/tests/Spice86.Tests/MachineLeakGuard.cs b/tests/Spice86.Tests/MachineLeakGuard.cs new file mode 100644 index 0000000000..77a9d88d1b --- /dev/null +++ b/tests/Spice86.Tests/MachineLeakGuard.cs @@ -0,0 +1,24 @@ +[assembly: Xunit.AssemblyFixture(typeof(Spice86.Tests.MachineLeakGuard))] + +namespace Spice86.Tests; + +using Xunit; + +/// +/// Assembly-wide guard that runs once after every test in the assembly has completed. It fails the run if +/// any emulator instance created through is still reachable after disposal and +/// garbage collection, which would indicate a reference leak pinning the whole Machine graph. +/// +public sealed class MachineLeakGuard : IDisposable { + public void Dispose() { + int tracked = MachineLeakTracker.TrackedCount; + if (tracked == 0) { + return; + } + + int surviving = MachineLeakTracker.CountSurvivingAfterCollection(); + Assert.True(surviving == 0, + $"{surviving} of {tracked} emulator instances created via Spice86Creator were still alive after " + + "disposal and garbage collection. This indicates a reference leak that keeps the entire Machine graph rooted."); + } +} diff --git a/tests/Spice86.Tests/MachineLeakTracker.cs b/tests/Spice86.Tests/MachineLeakTracker.cs new file mode 100644 index 0000000000..fe4c8fda40 --- /dev/null +++ b/tests/Spice86.Tests/MachineLeakTracker.cs @@ -0,0 +1,39 @@ +namespace Spice86.Tests; + +using Spice86.Core.Emulator.VM; + +/// +/// Tracks weak references to every created through . +/// A suite-wide check verifies that disposed emulator instances become collectible: a single rooted +/// Machine keeps its entire device, CFG and memory graph alive, and accumulating those across the suite +/// previously exhausted host memory. +/// +internal static class MachineLeakTracker { + private static readonly List TrackedMachines = new(); + private static readonly object SyncRoot = new(); + + public static void Track(Machine machine) { + lock (SyncRoot) { + TrackedMachines.Add(new WeakReference(machine)); + } + } + + public static int TrackedCount { + get { + lock (SyncRoot) { + return TrackedMachines.Count; + } + } + } + + public static int CountSurvivingAfterCollection() { + for (int i = 0; i < 3; i++) { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + lock (SyncRoot) { + return TrackedMachines.Count(reference => reference.IsAlive); + } + } +} diff --git a/tests/Spice86.Tests/Spice86.Tests.csproj b/tests/Spice86.Tests/Spice86.Tests.csproj index 4f6831015c..ba6f5926cf 100644 --- a/tests/Spice86.Tests/Spice86.Tests.csproj +++ b/tests/Spice86.Tests/Spice86.Tests.csproj @@ -4,6 +4,7 @@ enable enable nullable + true false