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
Add configurable, thread-safe resource and configuration management utilities with reference-counted resource handles and pluggable release strategies, and integrate them with existing logging.
New Features:
Introduce a generic resource management system with loaders, caching, reference-counted handles, and pluggable release strategies.
Add a configuration manager for type-safe key/value configs with JSON/file persistence and change watchers.
Define abstractions for configuration, resource management, resource loaders, handles, and release strategies for use across the framework.
Enhancements:
Route coroutine scheduler error reporting through the framework logging infrastructure instead of writing directly to the console.
Tests:
Add comprehensive unit tests covering resource manager loading, caching, concurrency, and release strategies.
Add comprehensive unit tests covering configuration manager CRUD operations, watchers, persistence, and concurrency behavior.
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!
Introduces a new thread-safe configuration system and resource management subsystem (with caching, reference-counted handles, and pluggable release strategies), wires them into abstractions and tests, and replaces coroutine error Console logging with the central ILogger-based logging.
Sequence diagram for resource loading and auto release strategy
sequenceDiagram
actor Client
participant ResourceManager
participant ResourceCache
participant IResourceLoader_T
participant ResourceHandle_T
participant IResourceReleaseStrategy
Client->>ResourceManager: LoadAsync_T_(path)
ResourceManager->>ResourceCache: Get_T_(path)
alt cached resource exists
ResourceCache-->>ResourceManager: cachedResource
ResourceManager->>ResourceCache: AddReference(path)
ResourceManager-->>Client: cachedResource
else not cached
ResourceManager->>IResourceLoader_T: LoadAsync(path)
IResourceLoader_T-->>ResourceManager: loadedResource
ResourceManager->>ResourceCache: Get_T_(path)
alt another thread cached
ResourceCache-->>ResourceManager: existingResource
ResourceManager->>IResourceLoader_T: Unload(loadedResource)
ResourceManager->>ResourceCache: AddReference(path)
ResourceManager-->>Client: existingResource
else still not cached
ResourceCache-->>ResourceManager: null
ResourceManager->>ResourceCache: Add(path, loadedResource)
ResourceManager->>ResourceCache: AddReference(path)
ResourceManager-->>Client: loadedResource
end
end
Client->>ResourceManager: GetHandle_T_(path)
ResourceManager->>ResourceCache: Get_T_(path)
ResourceCache-->>ResourceManager: resource
ResourceManager->>ResourceCache: AddReference(path)
ResourceManager-->>Client: ResourceHandle_T
Client->>ResourceHandle_T: Dispose()
ResourceHandle_T->>ResourceHandle_T: DisposeInternal()
ResourceHandle_T->>ResourceManager: onDispose(path)
ResourceManager->>ResourceCache: RemoveReference(path)
ResourceCache-->>ResourceManager: refCount
ResourceManager->>IResourceReleaseStrategy: ShouldRelease(path, refCount)
alt strategy allows release
IResourceReleaseStrategy-->>ResourceManager: true
ResourceManager->>ResourceCache: Get_object_(path)
ResourceCache-->>ResourceManager: resource
ResourceManager->>IResourceLoader_T: Unload(resource)
ResourceManager->>ResourceCache: Remove(path)
else strategy keeps resource
IResourceReleaseStrategy-->>ResourceManager: false
note over ResourceManager: Resource remains cached
end
Loading
Class diagram for the new resource management subsystem
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!
We reviewed changes in d88aa12...fa4c7ec on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
The reason will be displayed to describe this comment to others. Learn more.
Do not use literals that can be replaced with the ones from `Math` class
Constants such as pi, e, and tau are already defined in the Math class. Consider using them instead of manually defining the literal yourself. This makes it easier to read and follow the code.
The reason will be displayed to describe this comment to others. Learn more.
Do not use literals that can be replaced with the ones from `Math` class
Constants such as pi, e, and tau are already defined in the Math class. Consider using them instead of manually defining the literal yourself. This makes it easier to read and follow the code.
The reason will be displayed to describe this comment to others. Learn more.
Lambda's body can be simplified
If your lambda's body has a single statement, consider refactoring it to move away from block syntax to expression body. Doing so makes your code easier to read.
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.
Method with return type `Task` does not follow the naming convention
The consensus in .NET is to have names of methods dealing with asynchronous operations suffixed with Async. One such example is Stream.ReadAsync from System.IO. Doing so improves readability and provides crucial information at a glance.
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.
Lambda's body can be simplified
If your lambda's body has a single statement, consider refactoring it to move away from block syntax to expression body. Doing so makes your code easier to read.
帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
In ConfigurationManager.SetConfig the use of AddOrUpdate means oldValue is actually the new stored value, so the equality check will almost always succeed and watchers will never be notified; consider fetching the previous value (e.g., via TryGetValue before updating) so you can correctly detect changes and trigger notifications.
ConfigWatcherUnRegister is declared under the GFramework.Core.Abstractions.configuration namespace but implemented in the core project; aligning its namespace (or location) with the assembly it lives in would avoid cross-layer confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In ConfigurationManager.SetConfig the use of AddOrUpdate means `oldValue` is actually the new stored value, so the equality check will almost always succeed and watchers will never be notified; consider fetching the previous value (e.g., via TryGetValue before updating) so you can correctly detect changes and trigger notifications.
- ConfigWatcherUnRegister is declared under the GFramework.Core.Abstractions.configuration namespace but implemented in the core project; aligning its namespace (or location) with the assembly it lives in would avoid cross-layer confusion.
## Individual Comments### Comment 1
<locationpath="GFramework.Core/configuration/ConfigurationManager.cs"line_range="90-98" />
<code_context>
+ /// <summary>
+ /// 设置指定键的配置值
+ /// </summary>
+ public void SetConfig<T>(string key, T value)
+ {
+ if (string.IsNullOrWhiteSpace(key))+ throw new ArgumentException(KeyCannotBeNullOrEmptyMessage, nameof(key));++ var oldValue = _configs.AddOrUpdate(key, value!, (_, _) => value!);++ // 触发监听器+ if (!EqualityComparer<object>.Default.Equals(oldValue, value))+ {+ NotifyWatchers(key, value);
</code_context>
<issue_to_address>
**issue (bug_risk):** SetConfig never notifies watchers because `oldValue` is actually the new value returned by AddOrUpdate.
`ConcurrentDictionary.AddOrUpdate` returns the updated value, so `oldValue` will always be the new value and the comparison will never trigger `NotifyWatchers`. You need access to the pre-update value instead. For example, capture the previous value with `TryGetValue` before calling `AddOrUpdate`, or perform a `TryGetValue` after updating and compare that stored old value against the new one before deciding whether to notify watchers.
</issue_to_address>
### Comment 2
<locationpath="GFramework.Core/resource/ResourceManager.cs"line_range="72-81" />
<code_context>
+ {
+ var tempFile = Path.GetTempFileName();++ try+ {+ _configManager.SetConfig("key1", "value1");
</code_context>
<issue_to_address>
**suggestion:** Logging only the exception message in Load/LoadAsync drops stack traces and makes diagnosing load failures harder.
In `Load` and `LoadAsync`, the catch blocks currently log only `ex.Message` and then return null. Please log the full exception instead (using the appropriate ILogger overload), for example:
```csharpcatch (Exceptionex)
{
_logger.Error(ex, $"[ResourceManager] Failed to load resource '{path}'");
returnnull;
}
```
This keeps the null-return behavior but preserves full diagnostic context (stack trace, inner exceptions, etc.).
</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.
Summary by Sourcery
添加可配置的、线程安全的资源和配置管理工具,支持带引用计数的资源句柄和可插拔的释放策略,并与现有日志系统集成。
New Features:
Enhancements:
Tests:
Original summary in English
Summary by Sourcery
Add configurable, thread-safe resource and configuration management utilities with reference-counted resource handles and pluggable release strategies, and integrate them with existing logging.
New Features:
Enhancements:
Tests: