feat(TcpSocket): remove SetDataPackageAdapter method#550
Merged
Conversation
Reviewer's GuideThis PR removes the legacy SetDataPackageAdapter API in favor of unified AddDataPackageAdapter overloads, refactors the internal caching and callback registration logic, and enhances data conversion error handling. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes the SetDataPackageAdapter method from the TcpSocket library and replaces it with AddDataPackageAdapter methods. The main purpose is to change from a "set" paradigm that clears existing adapters to an "add" paradigm that accumulates adapters, allowing multiple data package adapters to be registered.
- Removed
SetDataPackageAdaptermethod and its overloads - Added corresponding
AddDataPackageAdaptermethods with similar functionality - Updated all test cases to use the new
AddDataPackageAdaptermethods
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| TcpSocketFactoryTest.cs | Updates all test method calls from SetDataPackageAdapter to AddDataPackageAdapter |
| ITcpSocketClientExtensions.cs | Removes SetDataPackageAdapter methods, adds AddDataPackageAdapter methods and improves caching logic |
| DataConverter.cs | Minor code formatting improvements and error handling enhancement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider using a thread-safe collection (e.g., ConcurrentDictionary) or adding locks around Cache/EntityCache operations to avoid race conditions when registering/removing adapters concurrently.
- After removing adapters, you may want to remove the client key when its adapter list becomes empty to prevent memory leaks from unused client entries in Cache and EntityCache.
- Silently catching all exceptions in DataConverter.TryConvertTo can obscure parsing issues—consider logging or handling specific exceptions so conversion failures are visible during debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a thread-safe collection (e.g., ConcurrentDictionary) or adding locks around Cache/EntityCache operations to avoid race conditions when registering/removing adapters concurrently.
- After removing adapters, you may want to remove the client key when its adapter list becomes empty to prevent memory leaks from unused client entries in Cache and EntityCache.
- Silently catching all exceptions in DataConverter.TryConvertTo can obscure parsing issues—consider logging or handling specific exceptions so conversion failures are visible during debugging.
## Individual Comments
### Comment 1
<location> `src/extensions/BootstrapBlazor.TcpSocket/Extensions/ITcpSocketClientExtensions.cs:114` </location>
<code_context>
+ client.AddDataPackageAdapter(new DataPackageAdapter(handler), callback);
}
+ private static readonly Dictionary<ITcpSocketClient, List<(Func<ReadOnlyMemory<byte>, ValueTask> ReceivedCallback, Delegate EntityCallback)>> EntityCache = [];
+
/// <summary>
</code_context>
<issue_to_address>
Storing Delegate in EntityCache may introduce type safety risks.
Consider replacing 'Delegate' with a specific delegate type to ensure type safety and reduce the risk of runtime errors.
</issue_to_address>
### Comment 2
<location> `src/extensions/BootstrapBlazor.TcpSocket/Extensions/ITcpSocketClientExtensions.cs:168` </location>
<code_context>
+ {
+ if (EntityCache.TryGetValue(client, out var list))
+ {
+ var items = list.Where(i => i.EntityCallback.Equals(callback)).ToList();
+ foreach (var c in items)
+ {
</code_context>
<issue_to_address>
Using Delegate.Equals for callback matching may not reliably identify the correct callback.
Delegate.Equals may not match callbacks that are functionally identical but not the same instance. Consider using reference equality or another method to ensure accurate callback removal.
</issue_to_address>
### Comment 3
<location> `src/extensions/BootstrapBlazor.Socket/DataConverter/DataConverter.cs:32` </location>
<code_context>
- entity = ret ? v : default;
+ var ret = false;
+ entity = default;
+ try
+ {
+ var v = CreateEntity();
+ if (Parse(data, v))
+ {
+ entity = v;
+ ret = true;
+ }
+ }
+ catch { }
+
return ret;
</code_context>
<issue_to_address>
Swallowing all exceptions in TryConvertTo may obscure underlying issues.
Catching all exceptions without logging or handling can hinder debugging and mask critical errors. Please log the exception or catch only specific, expected exception types.
</issue_to_address>
### Comment 4
<location> `test/UnitTestTcpSocket/TcpSocketFactoryTest.cs:500` </location>
<code_context>
// 设置数据适配器
var adapter = new DataPackageAdapter(new FixLengthDataPackageHandler(7));
- client.SetDataPackageAdapter(adapter, buffer =>
+ var callback = new Func<ReadOnlyMemory<byte>, ValueTask>(buffer =>
{
// buffer 即是接收到的数据
</code_context>
<issue_to_address>
Consider adding a test for removing a data package adapter after adding it.
Adding a test for adapter removal will confirm that callbacks are not triggered post-removal, validating the new registration and deregistration logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Link issues
fixes #549
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Remove SetDataPackageAdapter methods in favor of AddDataPackageAdapter overloads, add support for multiple adapters and generic callbacks via caching, and improve data conversion error handling
New Features:
Enhancements:
_cachetoCacheand restructure callback registration with local delegatesTests: