You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Add abstractions and implementation for a grouped, token-based pause stack manager supporting nested pauses, scoped lifetimes, and pause state events.
Provide a Godot-specific pause handler that bridges pause stack state to SceneTree global pause.
Enhancements:
Normalize functional-related namespaces and test namespaces to the lowercased functional.* convention.
Tests:
Add extensive unit tests for PauseStackManager covering tokens, grouping, scoped pauses, handlers, events, cleanup, and concurrent operations.
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Add abstractions and implementation for a grouped, token-based pause stack manager supporting nested pauses, scoped lifetimes, and pause state events.
Provide a Godot-specific pause handler that bridges pause stack state to SceneTree global pause.
Enhancements:
Normalize functional-related namespaces and test namespaces to the lowercased functional.* convention.
Tests:
Add extensive unit tests for PauseStackManager covering tokens, grouping, scoped pauses, handlers, events, cleanup, and concurrent operations.
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Add abstractions and implementation for a grouped, token-based pause stack manager supporting nested pauses, scoped lifetimes, and pause state events.
Provide a Godot-specific pause handler that bridges pause stack state to SceneTree global pause.
Enhancements:
Normalize functional-related namespaces and test namespaces to the lowercased functional.* convention.
Tests:
Add extensive unit tests for PauseStackManager covering tokens, grouping, scoped pauses, handlers, events, cleanup, and concurrent operations.
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Add abstractions and implementation for a grouped, token-based pause stack manager supporting nested pauses, scoped lifetimes, and pause state events.
Provide a Godot-specific pause handler that bridges pause stack state to SceneTree global pause.
Enhancements:
Normalize functional-related namespaces and test namespaces to the lowercased functional.* convention.
Tests:
Add extensive unit tests for PauseStackManager covering tokens, grouping, scoped pauses, handlers, events, cleanup, and concurrent operations.
We reviewed changes in e391bab...0534a27 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
Implements a token- and group-based PauseStackManager with abstractions and Godot integration, plus normalizes functional-related namespaces/usings and adds comprehensive unit tests for pause behavior and concurrency.
Provide a concrete, thread-safe pause stack manager implementation with scoped pause support and internal data structures.
Implement PauseStackManager with per-group stacks, token map, and handler list using ReaderWriterLockSlim for concurrency control.
Implement Push/Pop to manage PauseEntry stacks, handle non-top token removal, and trigger notifications only on state transitions.
Expose query APIs for paused state, depth, and reasons, and support clearing per group or all groups with appropriate notifications.
Manage lifecycle via DestroyAsync and ObjectDisposedException checks to prevent use-after-destroy, and integrate logging for operations and handler failures.
Add internal PauseEntry class to represent stack entries and PauseScope disposable to provide using-based scoped pauses.
Integrate pause system with Godot by implementing a handler that maps global pause state to SceneTree.Paused.
Implement GodotPauseHandler that observes pause events and sets SceneTree.Paused based on the Global pause group.
Log SceneTree pause changes via GD.Print and respect handler Priority semantics.
GFramework.Godot/pause/GodotPauseHandler.cs
Normalize functional namespaces and test namespaces/usings to lower-case functional.* convention.
Change core functional namespaces from GFramework.Core.Functional* to GFramework.Core.functional* in Option, Result, async extensions, and function extensions.
Update test usings and namespaces to reference the new lower-case functional namespaces and match folder naming (extensions/functional).
Remove redundant using of the old Functional namespace where no longer needed.
Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
The reason will be displayed to describe this comment to others. Learn more.
Lock held in an improper manner
Trying to acquire a lock on a local variable is error-prone as different threads may end up locking on different instances of the same variable. Instead, you should be locking on a class field that is preferably designated as private and readonly, i.e. a dedicated object. Microsoft guidelines explicitly state that you should avoid locking on:
Please address the comments from this code review:
## Overall Comments- In `PauseStackManager`, `NotifyHandlers` (and thus `OnPauseStateChanged`) is invoked while holding the write lock in `Push`, `Pop`, `ClearGroup`, and `ClearAll`, which risks deadlocks or re-entrancy issues if handlers or subscribers call back into the manager; consider capturing the state under the lock, releasing it, then invoking handlers and events outside the lock.
-`DestroyAsync` currently only disposes the `ReaderWriterLockSlim` without clearing `_pauseStacks`, `_tokenMap`, or `_handlers`; if there is any chance the instance is used after destruction this will throw on the first lock operation—consider either clearing internal data and preventing further use explicitly or documenting/enforcing that no methods may be called after `DestroyAsync`.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="69-71" />
<code_context>
+ _logger.Debug($"Pause pushed: {reason} (Group: {group}, Depth: {stack.Count})");++ // 状态变化检测：从未暂停 → 暂停+ if (!wasPaused)+ {+ NotifyHandlers(group, true);+ }+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid invoking external handlers/events while holding the write lock to prevent deadlocks and contention.
These methods call `NotifyHandlers` while holding the `ReaderWriterLockSlim` write lock, but `NotifyHandlers` runs arbitrary user code (`IPauseHandler` and event subscribers). This risks deadlocks (e.g., if a handler calls back into `IPauseStackManager`) and extended lock contention. Instead, compute the state change and capture the handler list under the lock, then release the lock and invoke handlers/events afterwards to avoid re-entrancy inside the critical section.
</issue_to_address>
### Comment 2
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="25-27" />
<code_context>
+ /// 异步销毁方法，在组件销毁时调用。
+ /// </summary>
+ /// <returns>表示异步操作完成的任务。</returns>
+ public ValueTask DestroyAsync()
+ {
+ _lock.Dispose();+ return ValueTask.CompletedTask;+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify and enforce post-destruction usage semantics; disposing the lock without guarding other methods can cause runtime exceptions.
`DestroyAsync` only disposes `_lock`, while all other public methods keep using it without checks. Any call after destruction will throw `ObjectDisposedException` from `ReaderWriterLockSlim`, and concurrent use during `DestroyAsync` is data-racey. Either add a `_disposed` flag and have public APIs short-circuit (no-op or explicit error/logging), or clearly document and enforce that instances are never used after destruction. The current failure mode is implicit and hard to diagnose.
Suggested implementation:
```csharpprivatereadonlyList<IPauseHandler>_handlers=new();
privatereadonlyReaderWriterLockSlim_lock=new();
privatereadonlyILogger_logger=LoggerFactoryResolver.Provider.CreateLogger(nameof(PauseStackManager));
privatereadonlyDictionary<PauseGroup, Stack<PauseEntry>>_pauseStacks=new();
privatereadonlyDictionary<Guid, PauseEntry>_tokenMap=new();
/// <summary>/// 组件的销毁标记，一旦为 <c>true</c>，表示不再允许对暂停栈进行任何操作。/// </summary>privatebool_disposed;
/// <summary>/// 异步销毁方法，在组件销毁时调用。/// 调用后不允许再使用本实例的任何公开方法；后续调用将被忽略或记录日志，而不会抛出锁已释放异常。/// </summary>/// <returns>表示异步操作完成的任务。</returns>publicValueTaskDestroyAsync()
{
// 使用写锁保证销毁过程与并发访问互斥_lock.EnterWriteLock();
try
{
if (_disposed)
{
// 已经销毁则直接返回，避免重复 Dispose 造成异常returnValueTask.CompletedTask;
}
_disposed=true;
// 此处仅修改内部状态，不再对外暴露任何可用操作_pauseStacks.Clear();
_tokenMap.Clear();
_handlers.Clear();
}
finally
{
_lock.ExitWriteLock();
}
// 在不持有锁的情况下释放底层锁资源，避免死锁风险_lock.Dispose();
returnValueTask.CompletedTask;
}
```
为彻底落实“销毁后不再使用实例”的语义，并避免在其他公开方法中因锁已释放导致的 `ObjectDisposedException`，需要在文件中做以下补充修改（这些方法未在当前片段中展示，需在完整文件中按现有风格修改）：
1. 在类内部添加一个私有辅助方法，用于在所有公开 API 开头快速检查 `_disposed` 状态并做统一处理，例如：
- 如果偏向静默失败：直接返回（或返回默认值）并记录一条 `Warning` 日志；
- 如果偏向显式失败：抛出一个更易理解的异常（例如 `InvalidOperationException("PauseStackManager has been destroyed.")`），而不是由 `ReaderWriterLockSlim` 抛出 `ObjectDisposedException`。
示例（需要放在类体中合适位置）：
```csharpprivatevoidThrowIfDisposed()
{
if (_disposed)
{
_logger.LogWarning("尝试在 PauseStackManager 已销毁后访问其方法，将忽略该操作。");
thrownewInvalidOperationException("PauseStackManager 已被销毁，不能再使用。");
}
}
```
或者如果希望静默忽略，可以改成返回 `bool` 表示是否可继续执行：
```csharpprivateboolEnsureNotDisposed()
{
if (_disposed)
{
_logger.LogWarning("尝试在 PauseStackManager 已销毁后访问其方法，将忽略该操作。");
returnfalse;
}
returntrue;
}
```2. 在所有公开方法（例如推入/弹出暂停、注册/注销 `IPauseHandler`、查询当前暂停状态等）的开头，在尝试进入 `_lock` 之前调用上述辅助方法：
- 若使用 `ThrowIfDisposed()`：直接在方法顶部调用 `ThrowIfDisposed();`；
- 若使用 `EnsureNotDisposed()`：在方法顶部写 `if (!EnsureNotDisposed()) return;` 或 `return default;`（视方法返回类型而定）。
3. 如果类实现了其它生命周期接口（例如 `IDisposable` 或框架特定接口），需要保证它们与 `DestroyAsync` 的语义一致，通常做法：
-`Dispose()` 内部调用 `DestroyAsync().GetAwaiter().GetResult();` 或者提取共享的同步销毁逻辑；
- 文档注释中统一说明：一旦调用销毁方法/Dispose，实例不可再使用。
4. 如果外部调用方目前在销毁后仍可能持有并使用 `PauseStackManager` 实例，建议在调用点增加空/状态检查或更新生命周期管理（例如通过 DI 容器注册为 Scoped/Singleton，并在作用域结束前确保不再被访问）。
</issue_to_address>
帮助我变得更有用！请对每条评论点 👍 或 👎，我会根据这些反馈来改进评审质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
In PauseStackManager, NotifyHandlers (and thus OnPauseStateChanged) is invoked while holding the write lock in Push, Pop, ClearGroup, and ClearAll, which risks deadlocks or re-entrancy issues if handlers or subscribers call back into the manager; consider capturing the state under the lock, releasing it, then invoking handlers and events outside the lock.
DestroyAsync currently only disposes the ReaderWriterLockSlim without clearing _pauseStacks, _tokenMap, or _handlers; if there is any chance the instance is used after destruction this will throw on the first lock operation—consider either clearing internal data and preventing further use explicitly or documenting/enforcing that no methods may be called after DestroyAsync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In `PauseStackManager`, `NotifyHandlers` (and thus `OnPauseStateChanged`) is invoked while holding the write lock in `Push`, `Pop`, `ClearGroup`, and `ClearAll`, which risks deadlocks or re-entrancy issues if handlers or subscribers call back into the manager; consider capturing the state under the lock, releasing it, then invoking handlers and events outside the lock.
-`DestroyAsync` currently only disposes the `ReaderWriterLockSlim` without clearing `_pauseStacks`, `_tokenMap`, or `_handlers`; if there is any chance the instance is used after destruction this will throw on the first lock operation—consider either clearing internal data and preventing further use explicitly or documenting/enforcing that no methods may be called after `DestroyAsync`.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="69-71" />
<code_context>
+ _logger.Debug($"Pause pushed: {reason} (Group: {group}, Depth: {stack.Count})");++ // 状态变化检测：从未暂停 → 暂停+ if (!wasPaused)+ {+ NotifyHandlers(group, true);+ }+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid invoking external handlers/events while holding the write lock to prevent deadlocks and contention.
These methods call `NotifyHandlers` while holding the `ReaderWriterLockSlim` write lock, but `NotifyHandlers` runs arbitrary user code (`IPauseHandler` and event subscribers). This risks deadlocks (e.g., if a handler calls back into `IPauseStackManager`) and extended lock contention. Instead, compute the state change and capture the handler list under the lock, then release the lock and invoke handlers/events afterwards to avoid re-entrancy inside the critical section.
</issue_to_address>
### Comment 2
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="25-27" />
<code_context>
+ /// 异步销毁方法，在组件销毁时调用。
+ /// </summary>
+ /// <returns>表示异步操作完成的任务。</returns>
+ public ValueTask DestroyAsync()
+ {
+ _lock.Dispose();+ return ValueTask.CompletedTask;+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify and enforce post-destruction usage semantics; disposing the lock without guarding other methods can cause runtime exceptions.
`DestroyAsync` only disposes `_lock`, while all other public methods keep using it without checks. Any call after destruction will throw `ObjectDisposedException` from `ReaderWriterLockSlim`, and concurrent use during `DestroyAsync` is data-racey. Either add a `_disposed` flag and have public APIs short-circuit (no-op or explicit error/logging), or clearly document and enforce that instances are never used after destruction. The current failure mode is implicit and hard to diagnose.
Suggested implementation:
```csharpprivatereadonlyList<IPauseHandler>_handlers=new();
privatereadonlyReaderWriterLockSlim_lock=new();
privatereadonlyILogger_logger=LoggerFactoryResolver.Provider.CreateLogger(nameof(PauseStackManager));
privatereadonlyDictionary<PauseGroup, Stack<PauseEntry>>_pauseStacks=new();
privatereadonlyDictionary<Guid, PauseEntry>_tokenMap=new();
/// <summary>/// 组件的销毁标记，一旦为 <c>true</c>，表示不再允许对暂停栈进行任何操作。/// </summary>privatebool_disposed;
/// <summary>/// 异步销毁方法，在组件销毁时调用。/// 调用后不允许再使用本实例的任何公开方法；后续调用将被忽略或记录日志，而不会抛出锁已释放异常。/// </summary>/// <returns>表示异步操作完成的任务。</returns>publicValueTaskDestroyAsync()
{
// 使用写锁保证销毁过程与并发访问互斥_lock.EnterWriteLock();
try
{
if (_disposed)
{
// 已经销毁则直接返回，避免重复 Dispose 造成异常returnValueTask.CompletedTask;
}
_disposed=true;
// 此处仅修改内部状态，不再对外暴露任何可用操作_pauseStacks.Clear();
_tokenMap.Clear();
_handlers.Clear();
}
finally
{
_lock.ExitWriteLock();
}
// 在不持有锁的情况下释放底层锁资源，避免死锁风险_lock.Dispose();
returnValueTask.CompletedTask;
}
```
为彻底落实“销毁后不再使用实例”的语义，并避免在其他公开方法中因锁已释放导致的 `ObjectDisposedException`，需要在文件中做以下补充修改（这些方法未在当前片段中展示，需在完整文件中按现有风格修改）：
1. 在类内部添加一个私有辅助方法，用于在所有公开 API 开头快速检查 `_disposed` 状态并做统一处理，例如：
- 如果偏向静默失败：直接返回（或返回默认值）并记录一条 `Warning` 日志；
- 如果偏向显式失败：抛出一个更易理解的异常（例如 `InvalidOperationException("PauseStackManager has been destroyed.")`），而不是由 `ReaderWriterLockSlim` 抛出 `ObjectDisposedException`。
示例（需要放在类体中合适位置）：
```csharpprivatevoidThrowIfDisposed()
{
if (_disposed)
{
_logger.LogWarning("尝试在 PauseStackManager 已销毁后访问其方法，将忽略该操作。");
thrownewInvalidOperationException("PauseStackManager 已被销毁，不能再使用。");
}
}
```
或者如果希望静默忽略，可以改成返回 `bool` 表示是否可继续执行：
```csharpprivateboolEnsureNotDisposed()
{
if (_disposed)
{
_logger.LogWarning("尝试在 PauseStackManager 已销毁后访问其方法，将忽略该操作。");
returnfalse;
}
returntrue;
}
```2. 在所有公开方法（例如推入/弹出暂停、注册/注销 `IPauseHandler`、查询当前暂停状态等）的开头，在尝试进入 `_lock` 之前调用上述辅助方法：
- 若使用 `ThrowIfDisposed()`：直接在方法顶部调用 `ThrowIfDisposed();`；
- 若使用 `EnsureNotDisposed()`：在方法顶部写 `if (!EnsureNotDisposed()) return;` 或 `return default;`（视方法返回类型而定）。
3. 如果类实现了其它生命周期接口（例如 `IDisposable` 或框架特定接口），需要保证它们与 `DestroyAsync` 的语义一致，通常做法：
-`Dispose()` 内部调用 `DestroyAsync().GetAwaiter().GetResult();` 或者提取共享的同步销毁逻辑；
- 文档注释中统一说明：一旦调用销毁方法/Dispose，实例不可再使用。
4. 如果外部调用方目前在销毁后仍可能持有并使用 `PauseStackManager` 实例，建议在调用点增加空/状态检查或更新生命周期管理（例如通过 DI 容器注册为 Scoped/Singleton，并在作用域结束前确保不再被访问）。
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Please address the comments from this code review:
## Overall Comments- In `PauseStackManager`, `NotifyHandlers` is invoked while holding the write lock (e.g., from `Push`, `ClearGroup`, `ClearAll`), which can easily lead to deadlocks if handlers or event subscribers call back into the manager; consider capturing a snapshot of handlers and the `OnPauseStateChanged` delegate under lock, releasing the lock, and only then invoking them.
-`DestroyAsync` disposes the internal `ReaderWriterLockSlim` but does not prevent subsequent calls to methods like `Push`/`Pop`, which will then throw `ObjectDisposedException`; if this is a lifecycle method, it may be safer to mark the manager as disposed and short‑circuit public methods (or document that it must not be used after destruction).
- Handler invocation order is recomputed via `OrderBy(h => h.Priority)` on every state change; if pause events are frequent and handlers are relatively stable, you could maintain the `_handlers` list in priority order on registration to avoid repeated sorting on each notification.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="68-71" />
<code_context>
++ _logger.Debug($"Pause pushed: {reason} (Group: {group}, Depth: {stack.Count})");++ // 状态变化检测：从未暂停 → 暂停+ if (!wasPaused)+ {+ NotifyHandlers(group, true);+ }+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling NotifyHandlers while holding the write-lock to prevent deadlocks and handler-induced contention.
Push, Pop, ClearGroup and ClearAll currently invoke NotifyHandlers while holding the ReaderWriterLockSlim write-lock. Because NotifyHandlers calls arbitrary user code (IPauseHandler implementations and event subscribers), this can cause deadlocks or long lock hold times if that code re-enters PauseStackManager or blocks.
Instead, under the lock, compute whether a notification is needed and capture a snapshot of the handlers/event delegate; then release the lock and invoke them using the snapshot. This keeps internal state thread-safe while avoiding lock re-entrancy and reducing the impact of external code on the lock duration.
</issue_to_address>
### Comment 2
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="25-27" />
<code_context>
+ /// 异步销毁方法，在组件销毁时调用。
+ /// </summary>
+ /// <returns>表示异步操作完成的任务。</returns>
+ public ValueTask DestroyAsync()
+ {
+ _lock.Dispose();+ return ValueTask.CompletedTask;+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using the manager after DestroyAsync will throw due to the disposed ReaderWriterLockSlim; consider adding a disposal guard.
Since DestroyAsync disposes the ReaderWriterLockSlim but other methods don’t check for disposal, any later call into Push/Pop/IsPaused/etc. will try to take a disposed lock and can throw ObjectDisposedException. If the lifecycle truly guarantees no calls after DestroyAsync, this might be acceptable but is fragile.
To harden this, consider introducing a `_disposed` flag set in DestroyAsync and checking it at the start of each public method, either throwing a clear ObjectDisposedException or no-op’ing where appropriate. Alternatively, if DestroyAsync is only a logical teardown, you could avoid disposing the lock explicitly and rely on GC instead.
Suggested implementation:
```csharpprivatereadonlyList<IPauseHandler>_handlers=new();
privatereadonlyReaderWriterLockSlim_lock=new();
privatebool_disposed;
privatereadonlyILogger_logger=LoggerFactoryResolver.Provider.CreateLogger(nameof(PauseStackManager));
``````csharp/// <summary>/// 异步销毁方法，在组件销毁时调用。/// </summary>/// <returns>表示异步操作完成的任务。</returns>publicValueTaskDestroyAsync()
{
if (_disposed)
{
returnValueTask.CompletedTask;
}
_disposed=true;
_lock.Dispose();
returnValueTask.CompletedTask;
}
```
To fully implement the disposal guard and avoid using a disposed `ReaderWriterLockSlim`, you should:
1. At the very beginning of each public method that uses `_lock` or other internal state (e.g. `Push(...)`, `Pop(...)`, `IsPaused(...)`, any `Try...` methods, etc.), add a guard like:
```csharpif (_disposed)
{
thrownewObjectDisposedException(nameof(PauseStackManager));
}
```
or, if more appropriate for some methods (e.g. query methods), you can return a safe default instead of throwing.
2. If there are any async methods or callbacks that might be scheduled to run after `DestroyAsync`, ensure they either:
- Check `_disposed` before taking `_lock`, or
- Are cancelled/unsubscribed as part of `DestroyAsync`.
3. If there is an interface (e.g. `IPauseStackManager`) that mandates `DestroyAsync`, consider documenting in XML comments that calling any other member after `DestroyAsync` results in `ObjectDisposedException`, to make the lifecycle contract explicit.
</issue_to_address>
请帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据反馈改进后续的 review。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
In PauseStackManager, NotifyHandlers is invoked while holding the write lock (e.g., from Push, ClearGroup, ClearAll), which can easily lead to deadlocks if handlers or event subscribers call back into the manager; consider capturing a snapshot of handlers and the OnPauseStateChanged delegate under lock, releasing the lock, and only then invoking them.
DestroyAsync disposes the internal ReaderWriterLockSlim but does not prevent subsequent calls to methods like Push/Pop, which will then throw ObjectDisposedException; if this is a lifecycle method, it may be safer to mark the manager as disposed and short‑circuit public methods (or document that it must not be used after destruction).
Handler invocation order is recomputed via OrderBy(h => h.Priority) on every state change; if pause events are frequent and handlers are relatively stable, you could maintain the _handlers list in priority order on registration to avoid repeated sorting on each notification.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In `PauseStackManager`, `NotifyHandlers` is invoked while holding the write lock (e.g., from `Push`, `ClearGroup`, `ClearAll`), which can easily lead to deadlocks if handlers or event subscribers call back into the manager; consider capturing a snapshot of handlers and the `OnPauseStateChanged` delegate under lock, releasing the lock, and only then invoking them.
-`DestroyAsync` disposes the internal `ReaderWriterLockSlim` but does not prevent subsequent calls to methods like `Push`/`Pop`, which will then throw `ObjectDisposedException`; if this is a lifecycle method, it may be safer to mark the manager as disposed and short‑circuit public methods (or document that it must not be used after destruction).
- Handler invocation order is recomputed via `OrderBy(h => h.Priority)` on every state change; if pause events are frequent and handlers are relatively stable, you could maintain the `_handlers` list in priority order on registration to avoid repeated sorting on each notification.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="68-71" />
<code_context>
++ _logger.Debug($"Pause pushed: {reason} (Group: {group}, Depth: {stack.Count})");++ // 状态变化检测：从未暂停 → 暂停+ if (!wasPaused)+ {+ NotifyHandlers(group, true);+ }+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling NotifyHandlers while holding the write-lock to prevent deadlocks and handler-induced contention.
Push, Pop, ClearGroup and ClearAll currently invoke NotifyHandlers while holding the ReaderWriterLockSlim write-lock. Because NotifyHandlers calls arbitrary user code (IPauseHandler implementations and event subscribers), this can cause deadlocks or long lock hold times if that code re-enters PauseStackManager or blocks.
Instead, under the lock, compute whether a notification is needed and capture a snapshot of the handlers/event delegate; then release the lock and invoke them using the snapshot. This keeps internal state thread-safe while avoiding lock re-entrancy and reducing the impact of external code on the lock duration.
</issue_to_address>
### Comment 2
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="25-27" />
<code_context>
+ /// 异步销毁方法，在组件销毁时调用。
+ /// </summary>
+ /// <returns>表示异步操作完成的任务。</returns>
+ public ValueTask DestroyAsync()
+ {
+ _lock.Dispose();+ return ValueTask.CompletedTask;+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using the manager after DestroyAsync will throw due to the disposed ReaderWriterLockSlim; consider adding a disposal guard.
Since DestroyAsync disposes the ReaderWriterLockSlim but other methods don’t check for disposal, any later call into Push/Pop/IsPaused/etc. will try to take a disposed lock and can throw ObjectDisposedException. If the lifecycle truly guarantees no calls after DestroyAsync, this might be acceptable but is fragile.
To harden this, consider introducing a `_disposed` flag set in DestroyAsync and checking it at the start of each public method, either throwing a clear ObjectDisposedException or no-op’ing where appropriate. Alternatively, if DestroyAsync is only a logical teardown, you could avoid disposing the lock explicitly and rely on GC instead.
Suggested implementation:
```csharpprivatereadonlyList<IPauseHandler>_handlers=new();
privatereadonlyReaderWriterLockSlim_lock=new();
privatebool_disposed;
privatereadonlyILogger_logger=LoggerFactoryResolver.Provider.CreateLogger(nameof(PauseStackManager));
``````csharp/// <summary>/// 异步销毁方法，在组件销毁时调用。/// </summary>/// <returns>表示异步操作完成的任务。</returns>publicValueTaskDestroyAsync()
{
if (_disposed)
{
returnValueTask.CompletedTask;
}
_disposed=true;
_lock.Dispose();
returnValueTask.CompletedTask;
}
```
To fully implement the disposal guard and avoid using a disposed `ReaderWriterLockSlim`, you should:
1. At the very beginning of each public method that uses `_lock` or other internal state (e.g. `Push(...)`, `Pop(...)`, `IsPaused(...)`, any `Try...` methods, etc.), add a guard like:
```csharpif (_disposed)
{
thrownewObjectDisposedException(nameof(PauseStackManager));
}
```
or, if more appropriate for some methods (e.g. query methods), you can return a safe default instead of throwing.
2. If there are any async methods or callbacks that might be scheduled to run after `DestroyAsync`, ensure they either:
- Check `_disposed` before taking `_lock`, or
- Are cancelled/unsubscribed as part of `DestroyAsync`.
3. If there is an interface (e.g. `IPauseStackManager`) that mandates `DestroyAsync`, consider documenting in XML comments that calling any other member after `DestroyAsync` results in `ObjectDisposedException`, to make the lifecycle contract explicit.
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
The reason will be displayed to describe this comment to others. Learn more.
Variable `token` is uninitialized
While some variables such as fields can be initialized and be assigned a default value, it is possible for local variables to not be initialized. This however is not a good practice as it is capable of critically altering your program's path, thereby affecting its logic.
It is therefore recommended that you always initialize variables, ideally where they're being declared.
The reason will be displayed to describe this comment to others. Learn more.
Variable `found` is uninitialized
While some variables such as fields can be initialized and be assigned a default value, it is possible for local variables to not be initialized. This however is not a good practice as it is capable of critically altering your program's path, thereby affecting its logic.
It is therefore recommended that you always initialize variables, ideally where they're being declared.
The reason will be displayed to describe this comment to others. Learn more.
Variable `pausedGroups` is uninitialized
While some variables such as fields can be initialized and be assigned a default value, it is possible for local variables to not be initialized. This however is not a good practice as it is capable of critically altering your program's path, thereby affecting its logic.
It is therefore recommended that you always initialize variables, ideally where they're being declared.
帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
NotifyHandlers iterates over _handlers without any synchronization while RegisterHandler/UnregisterHandler mutate the list under a write lock; consider taking a snapshot of _handlers under the lock (e.g., copy to an array) and iterating that snapshot to avoid potential race conditions or InvalidOperationException during concurrent modification.
DestroyAsync clears all pause state without emitting any OnPauseStateChanged notifications, which means handlers and subscribers won’t see the transition to an unpaused state; consider either calling ClearAll() internally or explicitly notifying affected groups before disposal if you want a consistent lifecycle signal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- NotifyHandlers iterates over _handlers without any synchronization while RegisterHandler/UnregisterHandler mutate the list under a write lock; consider taking a snapshot of _handlers under the lock (e.g., copy to an array) and iterating that snapshot to avoid potential race conditions or InvalidOperationException during concurrent modification.
- DestroyAsync clears all pause state without emitting any OnPauseStateChanged notifications, which means handlers and subscribers won’t see the transition to an unpaused state; consider either calling ClearAll() internally or explicitly notifying affected groups before disposal if you want a consistent lifecycle signal.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/pause/PauseStackManager.cs"line_range="421-426" />
<code_context>
+ /// </summary>
+ /// <paramname="group">发生状态变化的暂停组。</param>
+ /// <paramname="isPaused">新的暂停状态。</param>
+ private void NotifyHandlers(PauseGroup group, bool isPaused)
+ {
+ _logger.Debug($"Notifying handlers: Group={group}, IsPaused={isPaused}");++ // 按优先级排序后通知+ foreach (var handler in _handlers.OrderBy(h => h.Priority))+ {+ try
</code_context>
<issue_to_address>
**issue (bug_risk):** Enumerating _handlers without synchronization risks InvalidOperationException during concurrent register/unregister.
`NotifyHandlers` enumerates `_handlers` without a lock, while `RegisterHandler`/`UnregisterHandler` mutate `_handlers` under a write lock. Concurrent modification during enumeration can throw `InvalidOperationException`. To stay thread-safe without holding the lock while invoking handlers, take a snapshot under the lock (e.g. `var handlers = _handlers.OrderBy(h => h.Priority).ToArray();`) and iterate over that snapshot instead.
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
The reason will be displayed to describe this comment to others. Learn more.
Variable `pausedGroups` is uninitialized
While some variables such as fields can be initialized and be assigned a default value, it is possible for local variables to not be initialized. This however is not a good practice as it is capable of critically altering your program's path, thereby affecting its logic.
It is therefore recommended that you always initialize variables, ideally where they're being declared.
The reason will be displayed to describe this comment to others. Learn more.
Variable `handlersSnapshot` is uninitialized
While some variables such as fields can be initialized and be assigned a default value, it is possible for local variables to not be initialized. This however is not a good practice as it is capable of critically altering your program's path, thereby affecting its logic.
It is therefore recommended that you always initialize variables, ideally where they're being declared.
The reason will be displayed to describe this comment to others. Learn more.
Variable `handlersSnapshot` is uninitialized
While some variables such as fields can be initialized and be assigned a default value, it is possible for local variables to not be initialized. This however is not a good practice as it is capable of critically altering your program's path, thereby affecting its logic.
It is therefore recommended that you always initialize variables, ideally where they're being declared.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
GeWuYou
force-pushed
the
feature/pause-stack-manager
branch
from
Summary by Sourcery
引入一个支持分组的基于令牌的暂停栈管理系统，并集成 Godot，同时提供全面的测试以及函数式命名空间规范化。
New Features:
Enhancements:
functional.*约定。Tests:
PauseStackManager添加大量单元测试，覆盖令牌、分组、作用域暂停、处理器、事件、清理以及并发操作。Original summary in English
Summary by Sourcery
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
引入一个支持分组的基于令牌的暂停栈管理系统，并集成 Godot，同时提供全面的测试以及函数式命名空间规范化。
New Features:
Enhancements:
functional.*约定。Tests:
PauseStackManager添加大量单元测试，覆盖令牌、分组、作用域暂停、处理器、事件、清理以及并发操作。Original summary in English
Summary by Sourcery
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Enhancements:
Tests:
新特性：
IPauseStackManager、PauseToken、PauseGroup和IPauseHandler抽象，用于建模基于令牌、分组的暂停状态管理，并支持事件通知。PauseStackManager和PauseScope，用于按分组管理嵌套暂停栈，暴露暂停原因和深度，并提供基于作用域的暂停生命周期管理。GodotPauseHandler，用于在暂停栈管理器与 Godot 的SceneTree全局暂停状态之间建立桥接。改进：
functional.*命名约定。测试：
PauseStackManager单元测试，覆盖令牌有效性、嵌套与分组暂停、处理程序与事件回调、基于作用域的暂停、清理行为以及并发压栈/弹栈操作。Original summary in English
Summary by Sourcery
引入一个支持分组的基于令牌的暂停栈管理系统，并集成 Godot，同时提供全面的测试以及函数式命名空间规范化。
New Features:
Enhancements:
functional.*约定。Tests:
PauseStackManager添加大量单元测试，覆盖令牌、分组、作用域暂停、处理器、事件、清理以及并发操作。Original summary in English
Summary by Sourcery
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
引入一个支持分组的基于令牌的暂停栈管理系统，并集成 Godot，同时提供全面的测试以及函数式命名空间规范化。
New Features:
Enhancements:
functional.*约定。Tests:
PauseStackManager添加大量单元测试，覆盖令牌、分组、作用域暂停、处理器、事件、清理以及并发操作。Original summary in English
Summary by Sourcery
Introduce a token-based pause stack management system with group support and Godot integration, along with comprehensive tests and functional namespace normalization.
New Features:
Enhancements:
Tests: