Skip to content

Commit 3c5aeac

Browse files
IvanMurzakIvanMurzak
committed
Refactor Future API and tests to improve memory management and event handling
1 parent b92ae06 commit 3c5aeac

File tree

8 files changed

+99
-26
lines changed

8 files changed

+99
-26
lines changed

‎Assets/_PackageRoot/Runtime/Future/Future.API.cs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,13 @@ public IFuture<T> Canceled(Action action)
168168
Safe.Run(action, LogLevel);
169169
return this;
170170
}
171-
if (OnCanceled != null)
172-
OnCanceled += action;
171+
if (cleared)
172+
{
173+
if (LogLevel.IsActive(DebugLevel.Warning))
174+
Debug.LogWarning($"[ImageLoader] Future[id={Id}] Canceled event is not set because Future is cleared\n{Url}");
175+
return this;
176+
}
177+
OnCanceled += action;
173178
return this;
174179
}
175180

‎Assets/_PackageRoot/Runtime/Future/Future.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,14 @@ public partial class Future<T> : IFuture<T>, IFuture, IFutureInternal<T>, IDispo
2222
protected event Action<T> OnLoaded;
2323
protected event Action<Exception> OnFailedToLoad;
2424
protected event Action<bool> OnCompleted;
25-
protected WeakAction OnCanceled = new WeakAction();
25+
26+
// ┌─────────────────────┬────────────────────────────────────────────────┐
27+
// │ Memory Leak Warning │ Subscription on OnCanceled cross references │
28+
// └─────────────────────┘ with another Future │
29+
// │
30+
protected event Action OnCanceled; // │
31+
// protected WeakAction OnCanceled = new WeakAction(); // │
32+
// ───────────────────────────────────────────────────────────────────────┘
2633

2734
protected readonly CancellationTokenSource cts;
2835

‎Assets/_PackageRoot/Tests/Editor/TestFuture.Load.ThenCancel.cs‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,23 @@ public partial class TestFuture
99
[UnityTest] public IEnumerator LoadFrom_Source_ThenCancel_NoLogs() => TestUtils.RunNoLogs(LoadFrom_Source_ThenCancel);
1010
[UnityTest] public IEnumerator LoadFrom_Source_ThenCancel()
1111
{
12-
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: true);
13-
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: false);
14-
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: true);
15-
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: false);
12+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: true, useGC: true);
13+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: false, useGC: true);
14+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: true, useGC: true);
15+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: false, useGC: true);
16+
17+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: true, useGC: false);
18+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: false, useGC: false);
19+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: true, useGC: false);
20+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: false, useGC: false);
1621
}
17-
IEnumerator LoadFrom_Source_ThenCancel(bool useDiskCache, bool useMemoryCache)
22+
IEnumerator LoadFrom_Source_ThenCancel(bool useDiskCache, bool useMemoryCache, bool useGC)
1823
{
1924
ImageLoader.settings.useDiskCache = useDiskCache;
2025
ImageLoader.settings.useMemoryCache = useMemoryCache;
2126

2227
foreach (var url in TestUtils.ImageURLs)
23-
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.Source, FutureLoadedFrom.Source);
28+
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.Source, FutureLoadedFrom.Source, useGC);
2429

2530
yield return TestUtils.ClearEverything(message: null);
2631
}
@@ -34,7 +39,14 @@ [UnityTest] public IEnumerator LoadFrom_MemoryCache_ThenCancel()
3439
foreach (var url in TestUtils.ImageURLs)
3540
{
3641
yield return ImageLoader.LoadSprite(url).AsCoroutine();
37-
yield return TestUtils.LoadFromMemoryCacheThenCancel(url);
42+
yield return TestUtils.LoadFromMemoryCacheThenCancel(url, useGC: true);
43+
}
44+
// TODO: remove code duplicate
45+
yield return TestUtils.ClearEverything(message: null);
46+
foreach (var url in TestUtils.ImageURLs)
47+
{
48+
yield return ImageLoader.LoadSprite(url).AsCoroutine();
49+
yield return TestUtils.LoadFromMemoryCacheThenCancel(url, useGC: false);
3850
}
3951
}
4052

@@ -47,7 +59,14 @@ [UnityTest] public IEnumerator LoadFrom_DiskCache_ThenCancel()
4759
foreach (var url in TestUtils.ImageURLs)
4860
{
4961
yield return ImageLoader.LoadSprite(url).AsCoroutine();
50-
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.DiskCache, FutureLoadedFrom.DiskCache);
62+
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.DiskCache, FutureLoadedFrom.DiskCache, useGC: true);
63+
}
64+
// TODO: remove code duplicate
65+
yield return TestUtils.ClearEverything(message: null);
66+
foreach (var url in TestUtils.ImageURLs)
67+
{
68+
yield return ImageLoader.LoadSprite(url).AsCoroutine();
69+
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.DiskCache, FutureLoadedFrom.DiskCache, useGC: false);
5170
}
5271
}
5372
}

‎Assets/_PackageRoot/Tests/Editor/TestFutureWaitingAnotherFuture.Load.AndCancel.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
1+
using System.Linq;
12
using UnityEngine.TestTools;
23
using System.Collections;
34
using Extensions.Unity.ImageLoader.Tests.Utils;
5+
using UnityEngine;
46

57
namespace Extensions.Unity.ImageLoader.Tests
68
{
@@ -48,6 +50,7 @@ [UnityTest] public IEnumerator LoadFrom_DiskCache_AndCancel()
4850
ImageLoader.settings.useDiskCache = true;
4951
ImageLoader.settings.useMemoryCache = false;
5052

53+
// foreach (var url in TestUtils.ImageURLs.SelectMany(x => Enumerable.Repeat(x, 100)).OrderBy(x => Random.value))
5154
foreach (var url in TestUtils.ImageURLs)
5255
{
5356
yield return ImageLoader.LoadSprite(url).AsCoroutine();

‎Assets/_PackageRoot/Tests/Editor/TestFutureWaitingAnotherFuture.Load.ThenCancel.cs‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,25 @@ public partial class TestFutureWaitingAnotherFuture
99
[UnityTest] public IEnumerator LoadFrom_Source_ThenCancel_NoLogs() => TestUtils.RunNoLogs(LoadFrom_Source_ThenCancel);
1010
[UnityTest] public IEnumerator LoadFrom_Source_ThenCancel()
1111
{
12-
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: true);
13-
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: false);
14-
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: true);
15-
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: false);
12+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: true, useGC: true);
13+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: false, useGC: true);
14+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: true, useGC: true);
15+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: false, useGC: true);
16+
17+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: true, useGC: false);
18+
yield return LoadFrom_Source_ThenCancel(useDiskCache: true, useMemoryCache: false, useGC: false);
19+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: true, useGC: false);
20+
yield return LoadFrom_Source_ThenCancel(useDiskCache: false, useMemoryCache: false, useGC: false);
1621
}
17-
IEnumerator LoadFrom_Source_ThenCancel(bool useDiskCache, bool useMemoryCache)
22+
IEnumerator LoadFrom_Source_ThenCancel(bool useDiskCache, bool useMemoryCache, bool useGC)
1823
{
1924
ImageLoader.settings.useDiskCache = useDiskCache;
2025
ImageLoader.settings.useMemoryCache = useMemoryCache;
2126

2227
foreach (var url in TestUtils.ImageURLs)
2328
{
2429
ImageLoader.LoadSprite(url);
25-
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.Source, FutureLoadedFrom.Source);
30+
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.Source, FutureLoadedFrom.Source, useGC);
2631
}
2732

2833
yield return TestUtils.ClearEverything(message: null);
@@ -38,7 +43,15 @@ [UnityTest] public IEnumerator LoadFrom_MemoryCache_ThenCancel()
3843
{
3944
yield return ImageLoader.LoadSprite(url).AsCoroutine();
4045
ImageLoader.LoadSprite(url);
41-
yield return TestUtils.LoadFromMemoryCacheThenCancel(url);
46+
yield return TestUtils.LoadFromMemoryCacheThenCancel(url, useGC: true);
47+
}
48+
// TODO: remove code duplicate
49+
yield return TestUtils.ClearEverything(message: null);
50+
foreach (var url in TestUtils.ImageURLs)
51+
{
52+
yield return ImageLoader.LoadSprite(url).AsCoroutine();
53+
ImageLoader.LoadSprite(url);
54+
yield return TestUtils.LoadFromMemoryCacheThenCancel(url, useGC: false);
4255
}
4356
}
4457

@@ -52,7 +65,15 @@ [UnityTest] public IEnumerator LoadFrom_DiskCache_ThenCancel()
5265
{
5366
yield return ImageLoader.LoadSprite(url).AsCoroutine();
5467
ImageLoader.LoadSprite(url);
55-
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.DiskCache, FutureLoadedFrom.DiskCache);
68+
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.DiskCache, FutureLoadedFrom.DiskCache, useGC: true);
69+
}
70+
// TODO: remove code duplicate
71+
yield return TestUtils.ClearEverything(message: null);
72+
foreach (var url in TestUtils.ImageURLs)
73+
{
74+
yield return ImageLoader.LoadSprite(url).AsCoroutine();
75+
ImageLoader.LoadSprite(url);
76+
yield return TestUtils.LoadThenCancel(url, FutureLoadingFrom.DiskCache, FutureLoadedFrom.DiskCache, useGC: false);
5677
}
5778
}
5879
}

‎Assets/_PackageRoot/Tests/Editor/Utils/FutureListener.cs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
2-
31
using System.Collections.Generic;
42
using System.Linq;
53
using UnityEngine;

‎Assets/_PackageRoot/Tests/Editor/Utils/TestUtils.Load.cs‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,14 @@ public static IEnumerator Load(string url, FutureLoadingFrom? expectedLoadingFro
5656
future1.Dispose();
5757
yield return UniTask.Yield();
5858
}
59-
public static IEnumerator LoadFromMemoryCacheThenCancel(string url) => LoadThenCancel(url, null, FutureLoadedFrom.MemoryCache);
60-
public static IEnumerator LoadThenCancel(string url, FutureLoadingFrom? expectedLoadingFrom, FutureLoadedFrom expectedLoadedFrom)
59+
public static IEnumerator LoadFromMemoryCacheThenCancel(string url, bool useGC) => LoadThenCancel(url, null, FutureLoadedFrom.MemoryCache, useGC);
60+
61+
// public static IEnumerator LoadThenCancel(string url, FutureLoadingFrom? expectedLoadingFrom, FutureLoadedFrom expectedLoadedFrom)
62+
// {
63+
// yield return LoadThenCancel(url, expectedLoadingFrom, expectedLoadedFrom, useGC: true);
64+
// yield return LoadThenCancel(url, expectedLoadingFrom, expectedLoadedFrom, useGC: false);
65+
// }
66+
public static IEnumerator LoadThenCancel(string url, FutureLoadingFrom? expectedLoadingFrom, FutureLoadedFrom expectedLoadedFrom, bool useGC)
6167
{
6268
var future1 = ImageLoader.LoadSprite(url);
6369
var futureListener = future1.ToFutureListener();
@@ -71,6 +77,9 @@ public static IEnumerator LoadThenCancel(string url, FutureLoadingFrom? expected
7177

7278
futureListener.Assert_Events_NotContains(EventName.Canceled);
7379

80+
if (useGC)
81+
TestUtils.WaitForGCFast();
82+
7483
future1.Cancel();
7584

7685
if (expectedLoadingFrom.HasValue)
@@ -106,6 +115,11 @@ public static IEnumerator LoadThenCancel(string url, FutureLoadingFrom? expected
106115
}
107116
public static IEnumerator LoadFromMemoryCacheAndCancel(string url) => LoadAndCancel(url, null);
108117
public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedLoadingFrom)
118+
{
119+
yield return LoadAndCancel(url, expectedLoadingFrom, useGC: true);
120+
yield return LoadAndCancel(url, expectedLoadingFrom, useGC: false);
121+
}
122+
public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedLoadingFrom, bool useGC)
109123
{
110124
var future1 = ImageLoader.LoadSprite(url);
111125
var futureListener = future1.ToFutureListener();
@@ -115,6 +129,9 @@ public static IEnumerator LoadAndCancel(string url, FutureLoadingFrom? expectedL
115129
? expectedLoadingFrom.Value.ToEventName()
116130
: EventName.LoadedFromMemoryCache);
117131

132+
if (useGC)
133+
TestUtils.WaitForGCFast();
134+
118135
var task1 = future1.AsTask();
119136
future1.Cancel();
120137
var task2 = future1.AsTask();

‎Assets/_PackageRoot/Tests/Editor/Utils/TestUtils.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,18 @@ public static IEnumerator ClearEverything(string message)
3030
ImageLoader.ClearTextureRef();
3131
yield return ImageLoader.ClearCacheAll().AsUniTask().ToCoroutine();
3232

33+
WaitForGCFast();
34+
LogAssert.ignoreFailingMessages = false;
35+
}
36+
public static void WaitForGCFast()
37+
{
3338
GC.Collect(100, GCCollectionMode.Forced, blocking: true);
3439
GC.WaitForPendingFinalizers();
35-
LogAssert.ignoreFailingMessages = false;
3640
}
3741
public static IEnumerator WaitForGC(int millisecondsDelay = 100)
3842
{
3943
yield return UniTask.Delay(TimeSpan.FromMilliseconds(millisecondsDelay)).ToCoroutine();
40-
GC.Collect(100, GCCollectionMode.Forced, blocking: true);
41-
GC.WaitForPendingFinalizers();
44+
WaitForGCFast();
4245
yield return UniTask.Delay(TimeSpan.FromMilliseconds(millisecondsDelay)).ToCoroutine();
4346
}
4447
public static IEnumerator RunNoLogs(Func<IEnumerator> test)

0 commit comments

Comments
 (0)