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.Load.cs b/Unity-Package/Assets/root/Tests/Base/Utils/TestUtils.Load.cs index bf66e2c..37e8e96 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,40 @@ 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. + // + // 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, + FutureLoadingFrom? cancelAtLoadingFrom = null) { - 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 +51,51 @@ public static IEnumerator Load(string url, FutureLoadingFrom? expectedLoadingFro 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; + } + + 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 +146,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 +207,18 @@ 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); + // 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; - 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); 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(); }