From 1f2270bb5fc1832e2530213e49cc94ed577c70cf Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Thu, 28 May 2026 18:03:31 -0700 Subject: [PATCH 1/3] test: de-flake TestFutureWaitingAnotherFuture cancel-while-loading Two pre-existing flaky tests in the TestFutureWaitingAnotherFuture family were failing at random across the release matrix (mostly windows-mono legs), blocking the 7.0.4 publish. Both are test-harness races; runtime code is unchanged. 1) LoadingFromSource race (the reported flake) LoadFrom_Source_AndCancel -> LoadAndCancel(url, Source) starts an outer future loading the URL, then attaches an inner future that must observe LoadingFromSource and be cancelled mid-load. Because the in-process mock server answered instantly, the outer load often completed and populated the memory cache before the cancel, so the inner future observed LoadedFromMemoryCache instead -> "Expected event LoadingFromSource, but it was not found. Got: LoadedFromMemoryCache, Loaded, Completed". This was both the flake and a coverage gap (it sometimes cancelled after completion). Fix: add a deterministically held route to TestHttpServer (/hold/{id} gated by a per-id ManualResetEventSlim) plus HoldImage/ReleaseHeld/ ReleaseAllHeld, routed via MockWebRequestProvider.RegisterHeld and exposed to tests as TestUtils.BeginHold/ReleaseHeld. The cancel test now holds the source load in-flight across the whole LoadAndCancel sequence, so the inner future is guaranteed to be in LoadingFromSource when cancelled, then the hold is released so the outer load completes and cleans up. No timing guesses; assertions are unchanged. 2) Connection-reset race (uncovered while reproducing) TestHttpServer wrote the response and let `using` dispose the socket immediately. On Windows, closing with the peer's request bytes still unread makes the OS emit a TCP RST instead of FIN, so curl intermittently failed with "Curl error 56: Recv failure: Connection was reset", surfacing as "Failed to receive data" across many tests in the family. Fix: WriteAndClose now drains the request, half-closes the send side, and waits (bounded) for the peer FIN before disposing. Held workers are also unblocked on Stop() so no thread leaks. Local evidence: 2019.4.40f1 EditMode, filtered to TestFutureWaitingAnotherFuture. Baseline reproduced 9/20 failures; after the fix 10/10 consecutive runs were 20/20 green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Base/Utils/MockWebRequestProvider.cs | 23 ++- .../root/Tests/Base/Utils/TestHttpServer.cs | 142 +++++++++++++++++- .../Assets/root/Tests/Base/Utils/TestUtils.cs | 40 +++++ ...tureWaitingAnotherFuture.Load.AndCancel.cs | 8 + ...tureWaitingAnotherFuture.Load.AndCancel.cs | 8 + 5 files changed, 213 insertions(+), 8 deletions(-) diff --git a/Unity-Package/Assets/root/Tests/Base/Utils/MockWebRequestProvider.cs b/Unity-Package/Assets/root/Tests/Base/Utils/MockWebRequestProvider.cs index 44d3d7e..cdcb987 100644 --- a/Unity-Package/Assets/root/Tests/Base/Utils/MockWebRequestProvider.cs +++ b/Unity-Package/Assets/root/Tests/Base/Utils/MockWebRequestProvider.cs @@ -24,11 +24,30 @@ public static MockWebRequestProvider Instance } readonly Dictionary successUrlToImageId = new Dictionary(); + // URLs routed to the deterministically held route. The request reaches the + // server and stays in-flight until the test releases the matching id, which is + // what makes "cancel-while-loading" scenarios deterministic. + readonly Dictionary heldUrlToImageId = new Dictionary(); - public void Reset() => successUrlToImageId.Clear(); + public void Reset() + { + successUrlToImageId.Clear(); + heldUrlToImageId.Clear(); + } public void RegisterSuccess(string url, string imageId) => successUrlToImageId[url] = imageId; + /// + /// Routes to the server's held route (/hold/{imageId}). + /// The server must have the id armed via and + /// the test must release it via . While held, + /// a load of this URL stays in the LoadingFromSource state. Call + /// (or ) to restore the normal fast/slow routing. + /// + public void RegisterHeld(string url, string imageId) => heldUrlToImageId[url] = imageId; + + public void UnregisterHeld(string url) => heldUrlToImageId.Remove(url); + public UnityWebRequest CreateTextureRequest(string url) => UnityWebRequestTexture.GetTexture(ResolveUrl(url)); @@ -38,6 +57,8 @@ public UnityWebRequest CreateDataRequest(string url) string ResolveUrl(string url) { var baseUrl = TestHttpServer.Instance.BaseUrl; + if (heldUrlToImageId.TryGetValue(url, out var heldId)) + return $"{baseUrl}/hold/{heldId}"; return successUrlToImageId.TryGetValue(url, out var id) ? $"{baseUrl}/img/{id}" : $"{baseUrl}/slow"; diff --git a/Unity-Package/Assets/root/Tests/Base/Utils/TestHttpServer.cs b/Unity-Package/Assets/root/Tests/Base/Utils/TestHttpServer.cs index f570627..1eac5cc 100644 --- a/Unity-Package/Assets/root/Tests/Base/Utils/TestHttpServer.cs +++ b/Unity-Package/Assets/root/Tests/Base/Utils/TestHttpServer.cs @@ -9,12 +9,22 @@ namespace Extensions.Unity.ImageLoader.Tests.Utils { /// /// Minimal in-process HTTP server bound to 127.0.0.1 for deterministic, offline - /// image-loading tests. Serves pre-generated PNG bytes for success routes and a - /// deliberately slow route so the client-side Future timeout fires predictably. + /// image-loading tests. Serves pre-generated PNG bytes for success routes, a + /// deliberately slow route so the client-side Future timeout fires predictably, + /// and a deterministically *held* route whose response is blocked until the test + /// explicitly releases it. /// /// This replaces the public image URLs the tests used to fetch from GitHub, which /// were rate-limited and unreliable on CI, while keeping the loader's real /// UnityWebRequest send/decode path intact. + /// + /// The held route (/hold/{id}) exists to make "cancel-while-loading" + /// scenarios deterministic: the request reaches the server, the server begins a + /// response but parks the worker thread on a per-id gate, so the client-side + /// Future is guaranteed to be in the LoadingFromSource state (still + /// in-flight, not yet memory-cached) until the test calls . + /// This removes the timing race where a fast local load could complete before the + /// test got a chance to cancel it. /// public sealed class TestHttpServer { @@ -30,6 +40,10 @@ public static TestHttpServer Instance readonly object gate = new object(); readonly Dictionary images = new Dictionary(); + // Per-id gates for the /hold/{id} route. A held request parks on its event + // until the test releases it (or Stop() releases everything). Created lazily + // by HoldImage so a held route always serves a valid, registered image. + readonly Dictionary holdGates = new Dictionary(); TcpListener listener; Thread acceptThread; volatile bool running; @@ -63,6 +77,47 @@ public void RegisterImage(string id, byte[] pngBytes) lock (gate) images[id] = pngBytes; } + /// + /// Arms the held gate for . After this call any request to + /// /hold/{id} blocks (the connection stays open, response withheld) until + /// is called for the same id. Idempotent: re-holding an + /// already-armed id re-blocks it. The id must also be registered via + /// so a valid image is served once released. + /// + public void HoldImage(string id) + { + lock (gate) + { + if (holdGates.TryGetValue(id, out var existing)) + existing.Reset(); + else + holdGates[id] = new ManualResetEventSlim(initialState: false); + } + } + + /// + /// Releases a previously held id, letting its parked request(s) send the image + /// and complete. Safe to call when nothing is held (no-op). + /// + public void ReleaseHeld(string id) + { + ManualResetEventSlim ev; + lock (gate) holdGates.TryGetValue(id, out ev); + ev?.Set(); + } + + /// + /// Releases every held id. Used by and as a safety net in + /// test teardown so no worker thread is left parked. + /// + public void ReleaseAllHeld() + { + List all; + lock (gate) all = new List(holdGates.Values); + foreach (var ev in all) + ev.Set(); + } + public void Stop() { lock (gate) @@ -72,6 +127,8 @@ public void Stop() try { listener?.Stop(); } catch { /* already stopped */ } listener = null; } + // Unblock any parked /hold workers so they exit cleanly and threads don't leak. + ReleaseAllHeld(); } void AcceptLoop() @@ -104,7 +161,33 @@ void HandleClient(TcpClient client) { // Hold the response so the client-side timeout fires first. Thread.Sleep(SlowDelay); - TryWrite(stream, BuildResponse(200, "OK", "text/plain", Encoding.ASCII.GetBytes("slow"))); + WriteAndClose(client, stream, BuildResponse(200, "OK", "text/plain", Encoding.ASCII.GetBytes("slow"))); + return; + } + + if (path.StartsWith("/hold/")) + { + var id = path.Substring("/hold/".Length); + ManualResetEventSlim ev; + byte[] png; + bool found; + lock (gate) + { + holdGates.TryGetValue(id, out ev); + found = images.TryGetValue(id, out png); + } + // Park the worker until the test releases this id (or Stop() + // releases everything). The client request is fully sent and the + // connection is open, so the client-side Future is guaranteed to + // be in the LoadingFromSource state for the whole wait. + ev?.Wait(); + if (found && running) + { + WriteAndClose(client, stream, BuildResponse(200, "OK", "image/png", png)); + return; + } + // Released by Stop() (server shutting down) or id not registered: + // just drop the connection so the client errors out cleanly. return; } @@ -116,12 +199,12 @@ void HandleClient(TcpClient client) lock (gate) found = images.TryGetValue(id, out png); if (found) { - TryWrite(stream, BuildResponse(200, "OK", "image/png", png)); + WriteAndClose(client, stream, BuildResponse(200, "OK", "image/png", png)); return; } } - TryWrite(stream, BuildResponse(404, "Not Found", "text/plain", Encoding.ASCII.GetBytes("not found"))); + WriteAndClose(client, stream, BuildResponse(404, "Not Found", "text/plain", Encoding.ASCII.GetBytes("not found"))); } } catch { /* client aborted or disconnected — expected for slow/timeout tests */ } @@ -153,9 +236,54 @@ static byte[] BuildResponse(int code, string reason, string contentType, byte[] return response; } - static void TryWrite(NetworkStream stream, byte[] data) + // Writes the full response, then closes the connection *gracefully* to avoid a + // TCP RST. The original implementation wrote the bytes and let `using` dispose + // the socket immediately; on Windows, closing a socket while the peer's request + // bytes (headers/body) are still sitting unread in the receive buffer makes the + // OS send an RST instead of a FIN. curl then reports "Curl error 56: Recv + // failure: Connection was reset" at random, which surfaced as a flaky + // "Failed to receive data" test failure. The fix: drain the rest of the request, + // half-close our send side (sends FIN), then wait for the peer's FIN before + // disposing. This is the textbook lingering-close handshake. + static void WriteAndClose(TcpClient client, NetworkStream stream, byte[] data) { - try { stream.Write(data, 0, data.Length); stream.Flush(); } catch { /* client gone */ } + try + { + stream.Write(data, 0, data.Length); + stream.Flush(); + + var socket = client.Client; + // Drain whatever the client already sent (the rest of the HTTP request) + // so there is no unread inbound data when we close. + DrainAvailable(socket); + // Send FIN on our side; the client reads EOF and closes its side. + try { socket.Shutdown(SocketShutdown.Send); } catch { /* already closed */ } + // Wait for the client's FIN (ReadByte returns -1) so the close is fully + // graceful before the socket is disposed. Bounded so we never hang. + WaitForPeerClose(stream); + } + catch { /* client gone */ } + } + + static void DrainAvailable(Socket socket) + { + try + { + var buffer = new byte[1024]; + while (socket.Available > 0 && socket.Receive(buffer, 0, buffer.Length, SocketFlags.None) > 0) { } + } + catch { /* nothing more to drain */ } + } + + static void WaitForPeerClose(NetworkStream stream) + { + try + { + stream.ReadTimeout = 2000; + int b; + do { b = stream.ReadByte(); } while (b != -1); + } + catch { /* timeout or reset — close anyway */ } } } } diff --git a/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.cs b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.cs index 269983e..1a2a458 100644 --- a/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.cs +++ b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.cs @@ -116,6 +116,9 @@ public static Texture2D CreateTestTexture(int width = 64, int height = 64, Color public static void EnableMockProvider() { TestHttpServer.Instance.EnsureStarted(); + // Safety net: release any gate left armed by a previous (possibly failed) + // test so no server worker stays parked across tests. + TestHttpServer.Instance.ReleaseAllHeld(); if (mockPngCache == null) { @@ -141,5 +144,42 @@ public static void DisableMockProvider() { ImageLoader.settings.webRequestProvider = new DefaultWebRequestProvider(); } + + // Maps a registered image URL to its server-side image id (the index used by + // EnableMockProvider). Mirrors the registration in EnableMockProvider. + static string ImageIdForUrl(string url) + { + for (int i = 0; i < ImageURLs.Length; i++) + if (ImageURLs[i] == url) + return i.ToString(); + throw new ArgumentException($"URL is not a registered test image: {url}", nameof(url)); + } + + /// + /// Makes loading deterministically *hold* in-flight: the + /// request reaches the in-process server and parks there, so any Future loading + /// this URL stays in the LoadingFromSource state until + /// is called. This replaces racing the clock to catch a fast local load while it + /// is still running. Must be paired with . + /// + public static void BeginHold(string url) + { + var id = ImageIdForUrl(url); + TestHttpServer.Instance.EnsureStarted(); + TestHttpServer.Instance.HoldImage(id); + MockWebRequestProvider.Instance.RegisterHeld(url, id); + } + + /// + /// Releases a hold started by : the parked server response + /// is sent and the URL is routed back to the normal fast route. Safe to call even + /// if nothing is held. + /// + public static void ReleaseHeld(string url) + { + var id = ImageIdForUrl(url); + TestHttpServer.Instance.ReleaseHeld(id); + MockWebRequestProvider.Instance.UnregisterHeld(url); + } } } \ No newline at end of file diff --git a/Unity-Package/Assets/root/Tests/Editor/TestFutureWaitingAnotherFuture.Load.AndCancel.cs b/Unity-Package/Assets/root/Tests/Editor/TestFutureWaitingAnotherFuture.Load.AndCancel.cs index e3b9a6a..a22e97a 100644 --- a/Unity-Package/Assets/root/Tests/Editor/TestFutureWaitingAnotherFuture.Load.AndCancel.cs +++ b/Unity-Package/Assets/root/Tests/Editor/TestFutureWaitingAnotherFuture.Load.AndCancel.cs @@ -23,8 +23,16 @@ IEnumerator LoadFrom_Source_AndCancel(bool useDiskCache, bool useMemoryCache) foreach (var url in TestUtils.ImageURLs) { + // Hold the source load in-flight so the outer future stays in the + // LoadingFromSource state for the whole LoadAndCancel sequence. Without + // this, the fast in-process load can complete and populate the memory + // cache before the inner future is cancelled, so the inner future would + // observe LoadedFromMemoryCache instead of the LoadingFromSource cancel + // path under test — the source of the historical flake. + TestUtils.BeginHold(url); var future = ImageLoader.LoadSprite(url); yield return TestUtils.LoadAndCancel(url, FutureLoadingFrom.Source); + TestUtils.ReleaseHeld(url); future.Dispose(); } diff --git a/Unity-Package/Assets/root/Tests/Runtime/TestFutureWaitingAnotherFuture.Load.AndCancel.cs b/Unity-Package/Assets/root/Tests/Runtime/TestFutureWaitingAnotherFuture.Load.AndCancel.cs index e3b9a6a..a22e97a 100644 --- a/Unity-Package/Assets/root/Tests/Runtime/TestFutureWaitingAnotherFuture.Load.AndCancel.cs +++ b/Unity-Package/Assets/root/Tests/Runtime/TestFutureWaitingAnotherFuture.Load.AndCancel.cs @@ -23,8 +23,16 @@ IEnumerator LoadFrom_Source_AndCancel(bool useDiskCache, bool useMemoryCache) foreach (var url in TestUtils.ImageURLs) { + // Hold the source load in-flight so the outer future stays in the + // LoadingFromSource state for the whole LoadAndCancel sequence. Without + // this, the fast in-process load can complete and populate the memory + // cache before the inner future is cancelled, so the inner future would + // observe LoadedFromMemoryCache instead of the LoadingFromSource cancel + // path under test — the source of the historical flake. + TestUtils.BeginHold(url); var future = ImageLoader.LoadSprite(url); yield return TestUtils.LoadAndCancel(url, FutureLoadingFrom.Source); + TestUtils.ReleaseHeld(url); future.Dispose(); } From a0e2b2ba832db00928e338f4ebfce8510adcbace Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Thu, 28 May 2026 18:26:04 -0700 Subject: [PATCH 2/3] test: de-flake placeholder-consume in Load/LoadThenCancel/LoadAndCancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI surfaced a second pre-existing flake (TestFuture.LoadFrom_DiskCache_ThenCancel with usePlaceholder, on the fast 2019.4.40f1 playmode leg): Expected 6 events, but got 5. Expected: LoadingFromDiskCache, Consumed, LoadedFromDiskCache, Loaded, Consumed, Completed Got: LoadingFromDiskCache, LoadedFromDiskCache, Loaded, Consumed, Completed The first "Consumed" (the loading-state placeholder being applied) was missing. Root cause: the shared Load/LoadThenCancel/LoadAndCancel helpers did ImageLoader.LoadSprite(url) and only THEN called SetPlaceholder. LoadSprite starts the load synchronously inside the call, so on a fast disk/memory-cache read the future can reach LoadedFromDiskCache before the test registers the placeholders. When the LoadingFromDiskCache placeholder is not yet registered at the moment the loading event fires (and Status has already advanced when SetPlaceholder runs), the loading placeholder is never consumed and the expected first Consumed event never happens. Fix: register the placeholders BEFORE the load can advance. New helper StartLoadSpriteWithPlaceholders creates the future un-started (new FutureSprite), attaches the listener, registers the four placeholders, then calls StartLoading() — so the loading-state placeholder is guaranteed present when the loading event fires and is consumed deterministically. Behaviour for the non-placeholder path is unchanged (equivalent to ImageLoader.LoadSprite). Local evidence: 2019.4.40f1 EditMode, TestFuture.LoadFrom_DiskCache_ThenCancel 10/10 green after the fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../root/Tests/Base/Utils/TestUtils.Load.cs | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs index bf66e2c..a0a78d9 100644 --- a/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs +++ b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs @@ -8,11 +8,22 @@ namespace Extensions.Unity.ImageLoader.Tests.Utils { public static partial class TestUtils { - public static IEnumerator LoadFromMemoryCache(string url, bool usePlaceholder = false) => Load(url, null, FutureLoadedFrom.MemoryCache, usePlaceholder); - public static IEnumerator Load(string url, FutureLoadingFrom? expectedLoadingFrom, FutureLoadedFrom expectedLoadedFrom, bool usePlaceholder = false) + // Starts loading a sprite while guaranteeing that, when usePlaceholder is set, + // the loading-state placeholders are registered BEFORE the load can advance past + // the loading state. + // + // ImageLoader.LoadSprite(url) starts the load synchronously inside the call, so a + // fast (disk/memory-cache) load can reach LoadedFrom* before the test gets a + // chance to call SetPlaceholder afterwards. When that happens the loading-state + // placeholder is never consumed and the expected first "Consumed" event is + // missing — a timing flake (seen on fast CI runners). Creating the future + // un-started, attaching the listener, registering the placeholders, and only then + // calling StartLoading() makes the loading-placeholder consume deterministic. + static FutureListener StartLoadSpriteWithPlaceholders( + string url, bool usePlaceholder, bool ignoreLoadingWhenLoaded, out FutureSprite future) { - var future = ImageLoader.LoadSprite(url); - var futureListener = future.ToFutureListener(ignorePlaceholder: !usePlaceholder); + future = new FutureSprite(url); + var listener = future.ToFutureListener(ignoreLoadingWhenLoaded: ignoreLoadingWhenLoaded, ignorePlaceholder: !usePlaceholder); if (usePlaceholder) { @@ -22,6 +33,15 @@ public static IEnumerator Load(string url, FutureLoadingFrom? expectedLoadingFro future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.Canceled], PlaceholderTrigger.Canceled); } + future.StartLoading(); + return listener; + } + + public static IEnumerator LoadFromMemoryCache(string url, bool usePlaceholder = false) => Load(url, null, FutureLoadedFrom.MemoryCache, usePlaceholder); + public static IEnumerator Load(string url, FutureLoadingFrom? expectedLoadingFrom, FutureLoadedFrom expectedLoadedFrom, bool usePlaceholder = false) + { + var futureListener = StartLoadSpriteWithPlaceholders(url, usePlaceholder, ignoreLoadingWhenLoaded: false, out var future); + if (expectedLoadingFrom.HasValue) futureListener.Assert_Events_Contains(expectedLoadingFrom.Value.ToEventName()); @@ -72,16 +92,7 @@ public static IEnumerator LoadFromMemoryCacheThenCancel(string url, bool useGC, => LoadThenCancel(url, null, FutureLoadedFrom.MemoryCache, useGC, usePlaceholder); public static IEnumerator LoadThenCancel(string url, FutureLoadingFrom? expectedLoadingFrom, FutureLoadedFrom expectedLoadedFrom, bool useGC, bool usePlaceholder = false) { - var future = ImageLoader.LoadSprite(url); - var futureListener = future.ToFutureListener(ignorePlaceholder: !usePlaceholder); - - if (usePlaceholder) - { - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.LoadingFromDiskCache], PlaceholderTrigger.LoadingFromDiskCache); - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.LoadingFromSource], PlaceholderTrigger.LoadingFromSource); - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.FailedToLoad], PlaceholderTrigger.FailedToLoad); - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.Canceled], PlaceholderTrigger.Canceled); - } + var futureListener = StartLoadSpriteWithPlaceholders(url, usePlaceholder, ignoreLoadingWhenLoaded: false, out var future); if (expectedLoadingFrom.HasValue) futureListener.Assert_Events_Contains(expectedLoadingFrom.Value.ToEventName()); @@ -142,18 +153,9 @@ public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedL } public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedLoadingFrom, bool useGC, bool usePlaceholder = false) { - var future = ImageLoader.LoadSprite(url); - var futureListener = future.ToFutureListener(ignorePlaceholder: !usePlaceholder); + var futureListener = StartLoadSpriteWithPlaceholders(url, usePlaceholder, ignoreLoadingWhenLoaded: false, out var future); var shouldLoadFromMemoryCache = !expectedLoadingFrom.HasValue; - if (usePlaceholder) - { - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.LoadingFromDiskCache], PlaceholderTrigger.LoadingFromDiskCache); - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.LoadingFromSource], PlaceholderTrigger.LoadingFromSource); - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.FailedToLoad], PlaceholderTrigger.FailedToLoad); - future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.Canceled], PlaceholderTrigger.Canceled); - } - futureListener.Assert_Events_Contains(expectedLoadingFrom.HasValue ? expectedLoadingFrom.Value.ToEventName() : EventName.LoadedFromMemoryCache); From 347d1339feb9bbd0ae0218a1d19efb560518fbf5 Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Thu, 28 May 2026 18:55:52 -0700 Subject: [PATCH 3/3] test: de-flake cancel-while-loading-from-disk-cache in LoadAndCancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous PR run surfaced a third pre-existing flake on a fast windows-mono standalone leg: TestFuture.LoadFrom_DiskCache_AndCancel Expected: LoadingFromDiskCache, [Consumed,] Canceled, [Consumed,] Completed Got: LoadingFromDiskCache, [Consumed,] LoadedFromDiskCache, Loaded, [Consumed,] Completed Root cause: the disk-cache read is a local File.ReadAllBytes dispatched to a TaskFactory. On a fast machine that Task can finish and the awaiting continuation resume inline, so the future runs all the way to LoadedFromDiskCache *synchronously inside StartLoading()* — before the test's post-StartLoading Cancel() executes. The cancel then no-ops on an already-loaded future and the test sees a successful load instead of the cancel. Source loads never hit this (UnityWebRequest is genuinely async) and memory-cache is not a cancel-while-loading scenario, so only the disk-cache path is affected. Fix: for the disk-cache LoadAndCancel case, raise the Cancel() at the loading transition instead of after StartLoading(), via StartLoadSpriteWithPlaceholders' new cancelAtLoadingFrom hook. The hook is attached to the last synchronous loading-state callback so the event order is preserved exactly: - usePlaceholder == false: cancel from the LoadingFromDiskCache event -> LoadingFromDiskCache, Canceled, Completed - usePlaceholder == true: cancel from the loading-placeholder Consume (fires after the listener records the loading event and its consume, still in the loading state) -> LoadingFromDiskCache, Consumed, Canceled, Consumed, Completed The runtime re-checks IsCancelled after the disk read, so the cancel reliably wins. The explicit Cancel() later in LoadAndCancel becomes a harmless no-op. Assertions are unchanged. Local evidence: 2019.4.40f1 EditMode, TestFuture (68) and TestFutureWaitingAnotherFuture (20) full suites 3x each, all green; the disk-cancel tests now take the deterministic cancel path on every run. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../root/Tests/Base/Utils/TestUtils.Load.cs | 67 ++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs index a0a78d9..37e8e96 100644 --- a/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs +++ b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs @@ -19,8 +19,26 @@ public static partial class TestUtils // missing — a timing flake (seen on fast CI runners). Creating the future // un-started, attaching the listener, registering the placeholders, and only then // calling StartLoading() makes the loading-placeholder consume deterministic. + // + // cancelAtLoadingFrom makes "cancel-while-loading-from-disk-cache" deterministic. + // The disk read is a local File.ReadAllBytes dispatched to a TaskFactory; on a + // fast machine that Task can complete (and the awaiting continuation resume + // inline) entirely inside StartLoading(), so the future reaches + // LoadedFromDiskCache before the test's post-StartLoading Cancel() runs — the + // test then sees a successful load instead of the cancel (observed once on a + // windows-mono standalone leg). To cancel reliably *while in the loading state* + // we hook the cancel to the last synchronous loading-state callback so the + // event order is preserved exactly: + // - usePlaceholder == false: cancel from the LoadingFrom... event (no consume + // happens), yielding LoadingFrom..., Canceled, Completed. + // - usePlaceholder == true: cancel from the loading-placeholder Consume, which + // runs after the listener has recorded the loading event and its Consume, + // yielding LoadingFrom..., Consumed, Canceled, Consumed, Completed. + // The runtime re-checks IsCancelled after the disk read (Future.Loading.cs), so a + // cancel raised here reliably wins over the read result. static FutureListener StartLoadSpriteWithPlaceholders( - string url, bool usePlaceholder, bool ignoreLoadingWhenLoaded, out FutureSprite future) + string url, bool usePlaceholder, bool ignoreLoadingWhenLoaded, out FutureSprite future, + FutureLoadingFrom? cancelAtLoadingFrom = null) { future = new FutureSprite(url); var listener = future.ToFutureListener(ignoreLoadingWhenLoaded: ignoreLoadingWhenLoaded, ignorePlaceholder: !usePlaceholder); @@ -33,6 +51,42 @@ public static partial class TestUtils future.SetPlaceholder(placeholderSprites[PlaceholderTrigger.Canceled], PlaceholderTrigger.Canceled); } + if (cancelAtLoadingFrom.HasValue) + { + var self = future; + var loadingPlaceholderStatus = cancelAtLoadingFrom.Value == FutureLoadingFrom.Source + ? FutureStatus.LoadingFromSource + : FutureStatus.LoadingFromDiskCache; + var armed = true; + if (usePlaceholder) + { + // Cancel from the loading-placeholder consume (runs after the loading + // event + its consume have been recorded, while still in the loading + // state) so the consume/cancel order matches the expected sequence. + future.Consume(value => + { + if (armed && self.Status == loadingPlaceholderStatus) + { + armed = false; + self.Cancel(); + } + }); + } + else + { + void CancelOnce() + { + if (!armed) return; + armed = false; + self.Cancel(); + } + if (cancelAtLoadingFrom.Value == FutureLoadingFrom.Source) + future.LoadingFromSource(CancelOnce); + else + future.LoadingFromDiskCache(CancelOnce); + } + } + future.StartLoading(); return listener; } @@ -153,7 +207,16 @@ public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedL } public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedLoadingFrom, bool useGC, bool usePlaceholder = false) { - var futureListener = StartLoadSpriteWithPlaceholders(url, usePlaceholder, ignoreLoadingWhenLoaded: false, out var future); + // For a load from disk cache the read can complete synchronously inside + // StartLoading() and beat the explicit Cancel() below, so cancel at the + // loading transition instead (see StartLoadSpriteWithPlaceholders). The + // explicit Cancel() further down then becomes a harmless no-op. Source loads + // go through UnityWebRequest (never synchronous) and memory-cache loads are + // not a cancel-while-loading scenario, so neither needs this. + var cancelAtLoadingFrom = expectedLoadingFrom.HasValue && expectedLoadingFrom.Value == FutureLoadingFrom.DiskCache + ? expectedLoadingFrom + : null; + var futureListener = StartLoadSpriteWithPlaceholders(url, usePlaceholder, ignoreLoadingWhenLoaded: false, out var future, cancelAtLoadingFrom); var shouldLoadFromMemoryCache = !expectedLoadingFrom.HasValue; futureListener.Assert_Events_Contains(expectedLoadingFrom.HasValue