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
Improve core components’ thread-safety and error resilience across properties, events, logging, and coroutine scheduling.
Enhancements:
Make BindableProperty thread-safe with internal locking and invoke change callbacks outside locks to avoid deadlocks.
Add a timeout-based, more efficient Flush implementation to AsyncLogAppender and expose it via the ILogAppender interface.
Introduce a coroutine exception event and robust error handling in CoroutineScheduler to prevent scheduler crashes from user callbacks.
Switch EasyEvents to a thread-safe concurrent backing store and enforce single registration per event type, with clear errors on duplicates.
Harden FileAppender initialization with argument validation, safe writer setup, and cleanup on initialization failure.
Tests:
Add concurrency-focused unit tests for BindableProperty value setting, handler registration/unregistration, and RegisterWithInitValue.
Add multi-threaded unit tests for EasyEvents GetOrAdd and concurrent registration of different event types, plus validation of duplicate AddEvent behavior.
We reviewed changes in b417ece...718d7ca 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.
Refactors several core components to improve thread safety, error handling, and robustness, including BindableProperty, EasyEvents, AsyncLogAppender, CoroutineScheduler, FileAppender, and adds concurrent scenario tests.
Updated class diagram for core threading and logging components
Make BindableProperty thread-safe and avoid deadlocks when invoking callbacks.
Introduce a private lock object to guard value and delegate access.
Update Value setter to acquire the lock, check for value changes, update the backing field, capture the delegate, then invoke callbacks outside the lock.
Wrap Register and UnRegister modifications of the callback delegate in the same lock.
Change RegisterWithInitValue to read the current value under lock, invoke the callback with that snapshot, then register it for future changes.
Add concurrency-focused unit tests for value setting, event registering/triggering, register/unregister, and RegisterWithInitValue.
Add robust error handling for coroutine exceptions in CoroutineScheduler.
Introduce a public OnCoroutineException event to allow external handling of coroutine failures.
Update OnError to build a handle from the slot, invoke the OnCoroutineException callback in a try/catch to prevent callback failures from crashing the scheduler.
Log both callback exceptions and the original coroutine exception to Console.Error with clearer messages.
Ensure Complete is still called to finalize the failed coroutine slot.
Document that the scheduler is intended for single-threaded use.
GFramework.Core/coroutine/CoroutineScheduler.cs
Harden FileAppender initialization to clean up on failure and document error conditions.
Add XML documentation comments for constructor exceptions (ArgumentException for invalid path and IOException for file issues).
Wrap EnsureDirectoryExists and InitializeWriter in a try/catch in the constructor, disposing any partially created writer and nulling it before rethrowing.
Keep existing initialization behavior while ensuring resources are not leaked when initialization fails.
GFramework.Core/logging/appenders/FileAppender.cs
Tips and commands
Interacting with Sourcery
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.
Switch expression does not have `default` case
The default case is executed when none of the specified cases in the switch statement match. This is particularly useful when switch fails to cover all the possible values, either because all the cases weren't specified or that the range of values was later expanded.
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:
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:
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:
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:
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:
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:
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:
The reason will be displayed to describe this comment to others. Learn more.
Variable `currentValue` 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.
Please address the comments from this code review:
## Overall Comments- The new `AsyncLogAppender.Flush` loops on `_channel.Reader.Count` with `SpinWait`, which can both busy-spin the CPU for up to 30 seconds and still race with in‑flight reads (Count can be 0 while a consumer is processing); consider using a proper completion signal (e.g., waiting on a Task that completes when the consumer drains the channel) or `SpinWait.SpinUntil` combined with a more reliable condition.
- In `BindableProperty<T>`, only mutations and subscription changes are under `_lock` while `Value`'s `get` path and `GetValue()` remain unsynchronized, which can lead to torn reads or observing partially-updated state despite the class now being documented as fully thread-safe; consider taking the same lock on reads (or using a thread-safe backing mechanism) to match the advertised guarantees.
-`AsyncLogAppender` now returns a `bool` from `Flush(TimeSpan?)` but the `ILogAppender.Flush` explicit interface implementation discards this result, so callers via the interface cannot react to timeout/partial flush; if the timeout outcome is important, consider exposing it through the interface or providing another observable mechanism.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/coroutine/CoroutineScheduler.cs"line_range="39-43" />
<code_context>
+ /// <summary>
+ /// 协程异常处理回调，当协程执行过程中发生异常时触发
+ /// </summary>
+ public event Action<CoroutineHandle, Exception>? OnCoroutineException;
+
/// <summary>
</code_context>
<issue_to_address>
**suggestion (performance):** Invoking OnCoroutineException synchronously in OnError may let user callbacks block the scheduler loop.
Because `OnCoroutineException` is invoked directly from `OnError`, any slow or blocking subscriber will run on the scheduler’s path and can stall it, even though exceptions are caught.
To keep scheduler latency predictable, consider dispatching this event to another context (e.g., `Task.Run`, a dedicated error-handling queue, or a configurable dispatcher), or clearly document that `OnCoroutineException` handlers must be fast and non-blocking since they execute on the scheduler thread.
Suggested implementation:
```csharp/// <summary>/// 协程异常处理回调，当协程执行过程中发生异常时触发。/// 注意：事件处理程序会在独立任务中异步调用，以避免阻塞调度器主循环。/// </summary>publiceventAction<CoroutineHandle, Exception>?OnCoroutineException;
``````csharpvarhandler=OnCoroutineException;
if (handler!=null)
{
// 将回调调度到线程池，避免在调度器主循环中执行耗时或阻塞操作System.Threading.Tasks.Task.Run(() =>
{
try
{
handler(handle, exception);
}
catch
{
// 这里可以根据需要记录日志，但必须吞掉异常，避免影响调度器
}
});
}
```
If the file does not already reference `System.Threading.Tasks`, you may optionally add `using System.Threading.Tasks;` at the top and then shorten `System.Threading.Tasks.Task.Run` to `Task.Run`. Also ensure that the `SEARCH` block for `OnCoroutineException?.Invoke(handle, exception);` matches the actual invocation line in your `OnError` (or equivalent) method; adjust parameter order/names if they differ (e.g. `OnCoroutineException?.Invoke(coroutineHandle, ex);`).
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
帮助我变得更有用！请在每条评论上点击 👍 或 👎，我会根据这些反馈改进后续的审查建议。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
The new AsyncLogAppender.Flush loops on _channel.Reader.Count with SpinWait, which can both busy-spin the CPU for up to 30 seconds and still race with in‑flight reads (Count can be 0 while a consumer is processing); consider using a proper completion signal (e.g., waiting on a Task that completes when the consumer drains the channel) or SpinWait.SpinUntil combined with a more reliable condition.
In BindableProperty<T>, only mutations and subscription changes are under _lock while Value's get path and GetValue() remain unsynchronized, which can lead to torn reads or observing partially-updated state despite the class now being documented as fully thread-safe; consider taking the same lock on reads (or using a thread-safe backing mechanism) to match the advertised guarantees.
AsyncLogAppender now returns a bool from Flush(TimeSpan?) but the ILogAppender.Flush explicit interface implementation discards this result, so callers via the interface cannot react to timeout/partial flush; if the timeout outcome is important, consider exposing it through the interface or providing another observable mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- The new `AsyncLogAppender.Flush` loops on `_channel.Reader.Count` with `SpinWait`, which can both busy-spin the CPU for up to 30 seconds and still race with in‑flight reads (Count can be 0 while a consumer is processing); consider using a proper completion signal (e.g., waiting on a Task that completes when the consumer drains the channel) or `SpinWait.SpinUntil` combined with a more reliable condition.
- In `BindableProperty<T>`, only mutations and subscription changes are under `_lock` while `Value`'s `get` path and `GetValue()` remain unsynchronized, which can lead to torn reads or observing partially-updated state despite the class now being documented as fully thread-safe; consider taking the same lock on reads (or using a thread-safe backing mechanism) to match the advertised guarantees.
-`AsyncLogAppender` now returns a `bool` from `Flush(TimeSpan?)` but the `ILogAppender.Flush` explicit interface implementation discards this result, so callers via the interface cannot react to timeout/partial flush; if the timeout outcome is important, consider exposing it through the interface or providing another observable mechanism.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/coroutine/CoroutineScheduler.cs"line_range="39-43" />
<code_context>
+ /// <summary>
+ /// 协程异常处理回调，当协程执行过程中发生异常时触发
+ /// </summary>
+ public event Action<CoroutineHandle, Exception>? OnCoroutineException;
+
/// <summary>
</code_context>
<issue_to_address>
**suggestion (performance):** Invoking OnCoroutineException synchronously in OnError may let user callbacks block the scheduler loop.
Because `OnCoroutineException` is invoked directly from `OnError`, any slow or blocking subscriber will run on the scheduler’s path and can stall it, even though exceptions are caught.
To keep scheduler latency predictable, consider dispatching this event to another context (e.g., `Task.Run`, a dedicated error-handling queue, or a configurable dispatcher), or clearly document that `OnCoroutineException` handlers must be fast and non-blocking since they execute on the scheduler thread.
Suggested implementation:
```csharp/// <summary>/// 协程异常处理回调，当协程执行过程中发生异常时触发。/// 注意：事件处理程序会在独立任务中异步调用，以避免阻塞调度器主循环。/// </summary>publiceventAction<CoroutineHandle, Exception>?OnCoroutineException;
``````csharpvarhandler=OnCoroutineException;
if (handler!=null)
{
// 将回调调度到线程池，避免在调度器主循环中执行耗时或阻塞操作System.Threading.Tasks.Task.Run(() =>
{
try
{
handler(handle, exception);
}
catch
{
// 这里可以根据需要记录日志，但必须吞掉异常，避免影响调度器
}
});
}
```
If the file does not already reference `System.Threading.Tasks`, you may optionally add `using System.Threading.Tasks;` at the top and then shorten `System.Threading.Tasks.Task.Run` to `Task.Run`. Also ensure that the `SEARCH` block for `OnCoroutineException?.Invoke(handle, exception);` matches the actual invocation line in your `OnError` (or equivalent) method; adjust parameter order/names if they differ (e.g. `OnCoroutineException?.Invoke(coroutineHandle, ex);`).
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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}
Summary by Sourcery
在属性、事件、日志记录以及协程调度等核心组件中提升线程安全性和错误恢复能力。
增强点：
BindableProperty线程安全，同时在锁外调用变更回调以避免死锁。AsyncLogAppender添加基于超时的、更高效的Flush实现，并通过ILogAppender接口对外暴露。CoroutineScheduler中引入协程异常事件以及健壮的错误处理机制，防止用户回调导致调度器崩溃。EasyEvents切换为线程安全的并发存储结构，并强制每种事件类型仅允许单次注册，对于重复注册提供清晰的错误信息。FileAppender的初始化流程，包括参数校验、安全的 writer 设置，以及在初始化失败时进行清理。测试：
BindableProperty的数值设置、处理程序注册/反注册，以及RegisterWithInitValue添加以并发为重点的单元测试。EasyEvents的GetOrAdd和不同事件类型的多线程并发注册添加单元测试，并验证重复调用AddEvent时的行为。Original summary in English
Summary by Sourcery
Improve core components’ thread-safety and error resilience across properties, events, logging, and coroutine scheduling.
Enhancements:
Tests: