diff --git a/My project/Assets/FlowScope/Runtime/UI/UIPreloadService.cs b/My project/Assets/FlowScope/Runtime/UI/UIPreloadService.cs index 255eb79..6776850 100644 --- a/My project/Assets/FlowScope/Runtime/UI/UIPreloadService.cs +++ b/My project/Assets/FlowScope/Runtime/UI/UIPreloadService.cs @@ -13,6 +13,8 @@ namespace FlowScope.UI private readonly IResourceService _resources; private readonly Dictionary> _handles = new(); private readonly Dictionary _inFlightLoads = new(); + private int _releaseGeneration; + private bool _disposed; public UIPreloadService(IResourceService resources) { @@ -29,6 +31,8 @@ namespace FlowScope.UI string path = null; lock (_gate) { + ThrowIfDisposed(); + if (TryGetPreloadedHandle(panelType, out _)) { return; @@ -39,6 +43,7 @@ namespace FlowScope.UI var attribute = UIManager.GetPanelAttribute(panelType); path = UIManager.ResolvePath(panelType, attribute); inFlightLoad = new InFlightPreload(); + inFlightLoad.ReleaseGeneration = _releaseGeneration; _inFlightLoads.Add(panelType, inFlightLoad); startLoad = true; } @@ -65,6 +70,11 @@ namespace FlowScope.UI { lock (_gate) { + if (_disposed) + { + return false; + } + return TryGetPreloadedHandle(typeof(TPanel), out _); } } @@ -73,6 +83,12 @@ namespace FlowScope.UI { lock (_gate) { + if (_disposed) + { + handle = null; + return false; + } + return TryGetPreloadedHandle(typeof(TPanel), out handle); } } @@ -83,6 +99,11 @@ namespace FlowScope.UI IResourceHandle handle; lock (_gate) { + if (_disposed) + { + return; + } + if (!_handles.TryGetValue(panelType, out handle)) { return; @@ -99,6 +120,7 @@ namespace FlowScope.UI List> handles; lock (_gate) { + _releaseGeneration++; handles = new List>(_handles.Values); _handles.Clear(); } @@ -111,6 +133,16 @@ namespace FlowScope.UI public void Dispose() { + lock (_gate) + { + if (_disposed) + { + return; + } + + _disposed = true; + } + ReleaseAll(); } @@ -133,7 +165,9 @@ namespace FlowScope.UI lock (_gate) { _inFlightLoads.Remove(panelType); - if (inFlightLoad.WaiterCount > 0) + if (!_disposed && + inFlightLoad.ReleaseGeneration == _releaseGeneration && + inFlightLoad.WaiterCount > 0) { _handles[panelType] = handle; shouldCache = true; @@ -179,6 +213,14 @@ namespace FlowScope.UI handle.Asset != null; } + private void ThrowIfDisposed() + { + if (_disposed) + { + throw new ObjectDisposedException(nameof(UIPreloadService)); + } + } + private static async Task WaitForSharedLoadAsync( Task loadTask, CancellationToken cancellationToken) @@ -206,6 +248,7 @@ namespace FlowScope.UI { public Task Task { get; set; } public int WaiterCount { get; set; } + public int ReleaseGeneration { get; set; } } } } diff --git a/My project/Assets/FlowScope/Samples/MainMenuP0/Scripts/GameBootstrap.cs b/My project/Assets/FlowScope/Samples/MainMenuP0/Scripts/GameBootstrap.cs index e429adc..eb76c17 100644 --- a/My project/Assets/FlowScope/Samples/MainMenuP0/Scripts/GameBootstrap.cs +++ b/My project/Assets/FlowScope/Samples/MainMenuP0/Scripts/GameBootstrap.cs @@ -35,6 +35,9 @@ namespace FlowScope.Samples.MainMenuP0 private bool _shutdown; public int ReleasedResourceCount => _sampleBackend?.ReleaseCount ?? 0; + /// + /// True after runtime cleanup has completed; shutdown sub-step failures are still reported by ShutdownAsync. + /// public bool IsShutdown => _shutdown; public Task StartupTask { get; private set; } diff --git a/My project/Assets/FlowScope/Tests/PlayMode/UI/UIPreloadServiceTests.cs b/My project/Assets/FlowScope/Tests/PlayMode/UI/UIPreloadServiceTests.cs index 34bc66d..21c9327 100644 --- a/My project/Assets/FlowScope/Tests/PlayMode/UI/UIPreloadServiceTests.cs +++ b/My project/Assets/FlowScope/Tests/PlayMode/UI/UIPreloadServiceTests.cs @@ -155,6 +155,57 @@ namespace FlowScope.Tests.PlayMode.UI Assert.That(preload.IsPreloaded(), Is.False); } + [UnityTest] + public IEnumerator ReleaseAll_WhenLoadInFlight_DoesNotCacheAndDisposesLoadedHandle() + { + var prefab = CreatePrefab("ExplicitPathPrefab"); + var resources = new GatedUITestResources("Custom/Explicit", prefab); + var preload = new UIPreloadService(resources); + + var task = preload.PreloadAsync(CancellationToken.None); + Assert.That(resources.LoadCalls, Is.EqualTo(1)); + + preload.ReleaseAll(); + resources.Complete(); + yield return UITestAsync.Await(task); + + Assert.That(resources.Handle.IsDisposed, Is.True); + Assert.That(preload.IsPreloaded(), Is.False); + } + + [UnityTest] + public IEnumerator Dispose_WhenLoadInFlight_DoesNotCacheAndDisposesLoadedHandle() + { + var prefab = CreatePrefab("ExplicitPathPrefab"); + var resources = new GatedUITestResources("Custom/Explicit", prefab); + var preload = new UIPreloadService(resources); + + var task = preload.PreloadAsync(CancellationToken.None); + Assert.That(resources.LoadCalls, Is.EqualTo(1)); + + preload.Dispose(); + resources.Complete(); + yield return UITestAsync.Await(task); + + Assert.That(resources.Handle.IsDisposed, Is.True); + Assert.That(preload.IsPreloaded(), Is.False); + } + + [UnityTest] + public IEnumerator PreloadAsync_AfterDispose_ThrowsObjectDisposedException() + { + var prefab = CreatePrefab("ExplicitPathPrefab"); + var resources = new UITestResources(); + resources.Register("Custom/Explicit", prefab); + var preload = new UIPreloadService(resources); + + preload.Dispose(); + + yield return UITestAsync.AwaitExpected( + preload.PreloadAsync(CancellationToken.None)); + Assert.That(resources.LoadedKeys, Is.Empty); + } + [UnityTest] public IEnumerator OpenAsync_WhenPanelIsPreloaded_ReusesPreloadedHandle() { diff --git a/docs/reviews/p2-precheck-third-review.md b/docs/reviews/p2-precheck-third-review.md new file mode 100644 index 0000000..b8b8390 --- /dev/null +++ b/docs/reviews/p2-precheck-third-review.md @@ -0,0 +1,219 @@ +# FlowScope P2 前第三轮复审意见 + +日期：2026-05-21 +审查对象：`e211a71 修复 UI 预加载同步完成缓存问题` 及其前置整改提交 +审查范围：`GameBootstrap.ShutdownAsync`、`UIPreloadService` 并发/取消/释放语义、相关 PlayMode 测试、generated csproj 编译状态。 +审查结论：上一轮两个 P1 gate 已基本收口；当前进入 P2 前只剩一个 UI preload 生命周期边界建议先修。 + +--- + +## 本轮确认已收口 + +### 1. Shutdown best-effort 清理已落地 + +相关文件： + +- `My project/Assets/FlowScope/Samples/MainMenuP0/Scripts/GameBootstrap.cs` +- `My project/Assets/FlowScope/Tests/PlayMode/Samples/MainMenuP0Tests.cs` + +当前 `ShutdownAsync` 已经拆分： + +- `_shutdownInProgress` +- `_shutdown` +- `exceptions` + +并且按 best-effort 顺序执行： + +1. 尝试保存。 +2. 尝试关闭 `GameFlow`。 +3. 无论前面是否失败，仍尝试 `ReleaseAll()`。 +4. 最后再重新抛出单个异常或聚合异常。 + +对应新增测试： + +- `Shutdown_WhenSaveFails_StillShutsDownFlowAndReleasesPreloads` +- `Shutdown_WhenGameFlowShutdownFails_StillReleasesPreloads` +- `Shutdown_AfterFailure_DoesNotSkipCleanupOnSecondCall` + +评价：上一轮指出的“第一次 shutdown 失败后永久跳过后续清理”问题已基本修正。 + +--- + +### 2. UIPreloadService 首个调用者取消传播问题已修正 + +相关文件： + +- `My project/Assets/FlowScope/Runtime/UI/UIPreloadService.cs` +- `My project/Assets/FlowScope/Tests/PlayMode/UI/UIPreloadServiceTests.cs` + +当前实现变化： + +- `_inFlightLoads` 从 `Task` 改为 `InFlightPreload`。 +- 底层 `_resources.LoadAsync` 使用 `CancellationToken.None`。 +- 调用者自己的 token 只影响自己的等待。 +- `WaiterCount` 用来判断最终 handle 是否应缓存。 + +对应新增测试： + +- `PreloadAsync_WhenFirstWaiterCancels_SecondWaiterStillSucceeds` +- `PreloadAsync_WhenSecondWaiterCancels_FirstWaiterStillSucceeds` +- `PreloadAsync_WhenAllWaitersCancel_ReleasesLoadedHandleWithoutCaching` +- `PreloadAsync_WhenSamePanelLoadsConcurrently_UsesOneResourceLoad` + +评价：上一轮指出的“首个 caller 取消会影响其他 waiter”问题已修正。 + +--- + +### 3. 同步完成缓存问题已修正 + +最新提交 `e211a71 修复 UI 预加载同步完成缓存问题` 对 `UIPreloadService` 做了补充修正。 + +评价：这个修正方向是正确的，说明当前实现已经考虑到资源加载可能同步完成，不再只覆盖 gated async path。 + +--- + +## 仍建议 P2 前修正的问题 + +### P1. `UIPreloadService.ReleaseAll/Dispose` 未处理 in-flight preload + +风险等级：高 +建议：进入 P2 前修正。 + +证据文件： + +- `My project/Assets/FlowScope/Runtime/UI/UIPreloadService.cs` + +当前 `ReleaseAll()` 只处理 `_handles`： + +```csharp +public void ReleaseAll() +{ + List> handles; + lock (_gate) + { + handles = new List>(_handles.Values); + _handles.Clear(); + } + + foreach (var handle in handles) + { + handle.Dispose(); + } +} +``` + +但 `LoadAndStoreAsync()` 完成后仍可能写回 `_handles`： + +```csharp +lock (_gate) +{ + _inFlightLoads.Remove(panelType); + if (inFlightLoad.WaiterCount > 0) + { + _handles[panelType] = handle; + shouldCache = true; + } +} +``` + +问题场景： + +1. `PreloadAsync()` 已启动，底层资源仍在加载。 +2. 场景退出或 bootstrap shutdown 调用 `ReleaseAll()` / `Dispose()`。 +3. `ReleaseAll()` 清空已缓存 handle，但不会标记服务已释放，也不会处理 `_inFlightLoads`。 +4. 底层 load 随后完成。 +5. 因为 `WaiterCount > 0`，`LoadAndStoreAsync()` 仍会把 handle 写回 `_handles`。 + +结果： + +- 服务已经被释放后，异步完成又重新缓存资源。 +- shutdown 之后可能残留 handle。 +- P2 若加入 package/sample/editor tooling，这种生命周期反转会变成用户很难定位的泄漏或幽灵状态。 + +建议修法： + +1. 增加 `_disposed` 或 `_releaseGeneration` 状态。 +2. `ReleaseAll()` / `Dispose()` 时： + - 清空 `_handles`。 + - 标记当前 service 不再接受缓存写回。 + - 对 `_inFlightLoads` 做明确处理：要么取消 owner token，要么让完成后的 handle 直接 dispose。 +3. `PreloadAsync()` 在 service disposed 后应明确抛 `ObjectDisposedException` 或直接拒绝新请求，避免释放后继续使用。 +4. `LoadAndStoreAsync()` 完成时必须检查 release/dispose 状态；如果已释放，dispose handle，不缓存。 + +建议补测： + +- `ReleaseAll_WhenLoadInFlight_DoesNotCacheAndDisposesLoadedHandle` +- `Dispose_WhenLoadInFlight_DoesNotCacheAndDisposesLoadedHandle` +- `PreloadAsync_AfterDispose_ThrowsObjectDisposedException` + +--- + +## 中风险说明 + +### `IsShutdown` 语义需要避免误读 + +风险等级：中 +建议：可在 P2 前顺手明确文档或命名。 + +当前 `GameBootstrap.ShutdownAsync` 即使 `SaveAsync` 或 `GameFlow.ShutdownAsync` 抛错，也会在完成 best-effort cleanup 后设置： + +```csharp +_shutdown = true; +``` + +然后再向调用方抛出异常。 + +这个行为本身可以接受，因为它表达的是“runtime cleanup 已完成”，不是“所有 shutdown 子步骤都成功”。 +但 `IsShutdown` 容易被读成“关闭成功”。建议在样例文档或属性注释里明确： + +- `IsShutdown` 表示 MainMenuP0 runtime cleanup 已结束。 +- 不表示保存一定成功。 +- 调用者仍应以 `ShutdownAsync` 是否抛异常判断保存/关闭子步骤是否失败。 + +--- + +## 仍可后置的风险 + +### Addressables 真实 load/release 验证仍薄 + +这不是本轮整改新引入的问题，但仍是 P2 包分发前的验收缺口。 + +当前已有 generated csproj 编译覆盖，但 Addressables 适配层真实资源加载、释放、失败诊断仍缺少较强测试。 +如果 P2 先做 package 结构或文档，可以后置；如果 P2 要直接推广资源后端能力，建议补最小 PlayMode 集成或人工验收记录。 + +--- + +## 本轮验证 + +执行： + +```powershell +dotnet build "My project\FlowScope.Tests.EditMode.csproj" --no-restore +dotnet build "My project\FlowScope.Tests.PlayMode.csproj" --no-restore +``` + +结果： + +- EditMode generated csproj build：0 error。 +- PlayMode generated csproj build：0 error。 +- 仍有 Unity generated csproj 常见引用冲突 warning。 + +说明： + +- 本轮未执行 Unity Test Runner。 +- 本轮未做 MainMenuP0 人工场景验收。 + +--- + +## P2 Gate 判断 + +当前建议：暂缓进入 P2，先修一个最小问题。 + +P2 前最小清单： + +1. 修复 `UIPreloadService.ReleaseAll/Dispose` 与 in-flight preload 的生命周期边界。 +2. 补 in-flight release/dispose 对应 PlayMode 测试。 +3. 保持 EditMode / PlayMode generated csproj build 0 error。 + +完成后，P2 gate 可以认为基本通过。届时进入 P2 前只需再决定 Addressables 真实集成验收是 P2 前置，还是 P2 内第一项。 +