feat(ITcpSocketFactory): add ITcpSocketFactory service #6254
Conversation
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
Contributor
Reviewer's GuideAdds an ITcpSocketFactory service with DI support alongside ITcpSocketClient and IDataPackageHandler abstractions and default implementations, a fixed-length data handler, WebAssembly environment detection extension, and corresponding unit tests. Class diagram for new TCP socket abstractions and implementationsclassDiagram
class ITcpSocketFactory {
+ITcpSocketClient GetOrCreate(string host, int port)
+ITcpSocketClient? Remove(string host, int port)
}
class DefaultTcpSocketFactory {
+ITcpSocketClient GetOrCreate(string host, int port)
+ITcpSocketClient? Remove(string host, int port)
+void Dispose()
}
ITcpSocketFactory <|.. DefaultTcpSocketFactory
class ITcpSocketClient {
+int ReceiveBufferSize
+bool IsConnected
+IPEndPoint LocalEndPoint
+void SetDataHandler(IDataPackageHandler handler)
+Task<bool> ConnectAsync(string host, int port, CancellationToken token)
+Task<bool> ConnectAsync(IPEndPoint endPoint, CancellationToken token)
+Task<bool> SendAsync(Memory<byte> data, CancellationToken token)
+void Close()
}
class DefaultTcpSocketClient {
+int ReceiveBufferSize
+bool IsConnected
+IPEndPoint LocalEndPoint
+ILogger Logger
+void SetDataHandler(IDataPackageHandler handler)
+Task<bool> ConnectAsync(string host, int port, CancellationToken token)
+Task<bool> ConnectAsync(IPEndPoint endPoint, CancellationToken token)
+Task<bool> SendAsync(Memory<byte> data, CancellationToken token)
+void Close()
+void Dispose()
}
ITcpSocketClient <|.. DefaultTcpSocketClient
class IDataPackageHandler {
+Func<Memory<byte>, Task>? ReceivedCallBack
+Task<Memory<byte>> SendAsync(Memory<byte> data)
+Task ReceiveAsync(Memory<byte> data)
}
class DataPackageHandlerBase {
+Func<Memory<byte>, Task>? ReceivedCallBack
+Task<Memory<byte>> SendAsync(Memory<byte> data)
+Task ReceiveAsync(Memory<byte> data)
#void HandlerStickyPackage(Memory<byte> buffer, int length)
#Memory<byte> ConcatBuffer(Memory<byte> buffer)
}
IDataPackageHandler <|.. DataPackageHandlerBase
class FixLengthDataPackageHandler {
+FixLengthDataPackageHandler(int length)
+Task ReceiveAsync(Memory<byte> data)
}
DataPackageHandlerBase <|-- FixLengthDataPackageHandler
DefaultTcpSocketFactory o-- ITcpSocketClient
DefaultTcpSocketClient o-- IDataPackageHandler
FixLengthDataPackageHandler ..|> DataPackageHandlerBase
Class diagram for HostEnvironmentExtensions WebAssembly detectionclassDiagram
class HostEnvironmentExtensions {
+static bool IsWasm(IHostEnvironment hostEnvironment)
}
class IHostEnvironment
class MockWasmHostEnvironment
IHostEnvironment <|.. MockWasmHostEnvironment
HostEnvironmentExtensions ..> IHostEnvironment : uses
HostEnvironmentExtensions ..> MockWasmHostEnvironment : checks instance of
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- ConcatBuffer may return memory from a disposed pool block. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:67` </location>
<code_context>
+ await _client.ConnectAsync(endPoint, token);
+
+ // 开始接收数据
+ _ = Task.Run(ReceiveAsync, token);
+
+ LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
</code_context>
<issue_to_address>
Fire-and-forget ReceiveAsync may hide exceptions.
Since exceptions in fire-and-forget tasks like ReceiveAsync are unobserved, explicitly handle or log them to prevent crashes or lost errors.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:57` </location>
<code_context>
+ /// for the specified <paramref name="length"/>.</remarks>
+ /// <param name="buffer">The memory buffer containing the data to process.</param>
+ /// <param name="length">The length of the valid data within the buffer.</param>
+ protected void HandlerStickyPackage(Memory<byte> buffer, int length)
+ {
+ var len = buffer.Length - length;
</code_context>
<issue_to_address>
HandlerStickyPackage may leak pooled memory.
Since _lastReceiveBuffer is assigned a rented memory block that is disposed at the end of the method, this may lead to invalid memory access. Please ensure the data is copied to a new array or that the memory block's lifetime is managed appropriately.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:77` </location>
<code_context>
+ /// <param name="buffer">The buffer to concatenate with the previously stored data. Must not be empty.</param>
+ /// <returns>A <see cref="Memory{T}"/> instance containing the concatenated data. If no previously stored data exists, the
+ /// method returns the input <paramref name="buffer"/>.</returns>
+ protected Memory<byte> ConcatBuffer(Memory<byte> buffer)
+ {
+ if (_lastReceiveBuffer.IsEmpty)
</code_context>
<issue_to_address>
ConcatBuffer may return memory from a disposed pool block.
The method returns a Memory<byte> referencing a disposed pooled block, risking use-after-free. Copy the data to a new array or ensure the caller manages the pooled memory's lifetime.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:62` </location>
<code_context>
+ var len = buffer.Length - length;
+ if (len > 0)
+ {
+ using var memoryBlock = MemoryPool<byte>.Shared.Rent(len);
+ buffer[length..].CopyTo(memoryBlock.Memory);
+ _lastReceiveBuffer = memoryBlock.Memory[..len];
</code_context>
<issue_to_address>
Consider replacing the use of MemoryPool with direct array allocation or ArrayBufferWriter to simplify buffer management and avoid potential issues with disposed memory.
```csharp
// Suggestion: remove the pooled‐memory dance and just copy into a new array.
// This keeps all functionality but avoids renting/disposing and returning slices
// of already‐returned (disposed) memory.
protected void HandlerStickyPackage(Memory<byte> buffer, int length)
{
var leftover = buffer.Slice(length);
if (!leftover.IsEmpty)
{
// Copy leftover into a new array
_lastReceiveBuffer = leftover.ToArray().AsMemory();
}
}
protected Memory<byte> ConcatBuffer(Memory<byte> buffer)
{
if (_lastReceiveBuffer.IsEmpty)
{
return buffer;
}
var total = _lastReceiveBuffer.Length + buffer.Length;
var merged = new byte[total];
_lastReceiveBuffer.CopyTo(merged);
buffer.CopyTo(merged.AsMemory(_lastReceiveBuffer.Length));
// Clear the sticky buffer
_lastReceiveBuffer = Memory<byte>.Empty;
return merged;
}
```
If you still need pooling for high‐throughput scenarios, consider using an `ArrayBufferWriter<byte>` stored on the instance instead of `MemoryPool`. For example:
```csharp
private ArrayBufferWriter<byte> _writer = new ArrayBufferWriter<byte>();
protected void HandlerStickyPackage(Memory<byte> buffer, int length)
{
var leftover = buffer.Slice(length);
_writer.Clear();
_writer.Write(leftover.Span);
_lastReceiveBuffer = _writer.WrittenMemory;
}
protected Memory<byte> ConcatBuffer(Memory<byte> buffer)
{
if (_lastReceiveBuffer.IsEmpty)
return buffer;
_writer.Clear();
_writer.Write(_lastReceiveBuffer.Span);
_writer.Write(buffer.Span);
_lastReceiveBuffer = Memory<byte>.Empty;
return _writer.WrittenMemory;
}
```
Both approaches maintain the same behavior while removing fragile rent/dispose patterns.
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:15` </location>
<code_context>
+/// initialization. It extends <see cref="DataPackageHandlerBase"/> and overrides its behavior to process fixed-length
+/// data.</remarks>
+/// <param name="length">The data package total data length.</param>
+public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
+{
+ private readonly Memory<byte> _data = new byte[length];
</code_context>
<issue_to_address>
Consider simplifying the packet handling logic by using a single buffer and index in a loop to process multiple packets per call without extra buffer management methods.
Here’s a much simpler way to do exactly the same thing in a single buffer with an index, no `ConcatBuffer`/`HandlerStickyPackage` needed. It will also correctly handle multiple back-to-back packages in one `ReceiveAsync` call:
```csharp
public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
{
private readonly byte[] _buffer = new byte[length];
private int _offset;
public override Task ReceiveAsync(Memory<byte> data)
{
var span = data.Span;
int read = 0;
while (read < span.Length)
{
// how many bytes we still need to fill the packet
int need = length - _offset;
// how many bytes are available right now
int avail = span.Length - read;
// copy that many
int toCopy = Math.Min(need, avail);
span.Slice(read, toCopy).CopyTo(_buffer.AsSpan(_offset));
_offset += toCopy;
read += toCopy;
if (_offset == length)
{
// one full packet received
_offset = 0;
// fire the callback
if (ReceivedCallBack != null)
{
// note: can return directly if you prefer
_ = ReceivedCallBack(_buffer);
}
}
}
return Task.CompletedTask;
}
}
```
What this does:
1. Keeps exactly one `byte[length]` buffer – no rentals or concatenations
2. Copies in a tight `while` loop so any extra bytes automatically start building the *next* packet
3. Invokes your callback immediately when a full packet is ready
4. Drops all `ConcatBuffer`/`HandlerStickyPackage` ceremony entirely while preserving semantics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6254 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 704 710 +6
Lines 31137 31339 +202
Branches 4403 4424 +21
==========================================
+ Hits 31137 31339 +202
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- The DefaultTcpSocketFactory uses a primary‐constructor syntax (
class DefaultTcpSocketFactory(IServiceProvider provider)) that isn’t valid C#—you need a regular constructor that stores the IServiceProvider in a private field. - HostEnvironmentExtensions.IsWasm currently checks for the test‐only MockWasmHostEnvironment type, so it won’t ever detect real WebAssembly—replace it with a runtime check (e.g. RuntimeInformation) instead of a test class.
- DataPackageHandlerBase.ConcatBuffer and HandlerStickyPackage rent buffers from MemoryPool but never return them—switch to using IMemoryOwner and Dispose() to avoid memory leaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The DefaultTcpSocketFactory uses a primary‐constructor syntax (`class DefaultTcpSocketFactory(IServiceProvider provider)`) that isn’t valid C#—you need a regular constructor that stores the IServiceProvider in a private field.
- HostEnvironmentExtensions.IsWasm currently checks for the test‐only MockWasmHostEnvironment type, so it won’t ever detect real WebAssembly—replace it with a runtime check (e.g. RuntimeInformation) instead of a test class.
- DataPackageHandlerBase.ConcatBuffer and HandlerStickyPackage rent buffers from MemoryPool<byte> but never return them—switch to using IMemoryOwner<byte> and Dispose() to avoid memory leaks.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:162` </location>
<code_context>
+ Dispose(true);
+ }
+
+ private void Dispose(bool disposing)
+ {
+ if (disposing)
</code_context>
<issue_to_address>
Dispose pattern does not implement finalizer, but calls GC.SuppressFinalize.
Remove GC.SuppressFinalize if you don't intend to add a finalizer.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:17` </location>
<code_context>
+/// <param name="length">The data package total data length.</param>
+public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
+{
+ private readonly Memory<byte> _data = new byte[length];
+
+ private int _receivedLength;
</code_context>
<issue_to_address>
Using Memory<byte> for _data may lead to unexpected behavior if reused.
Ensure consumers do not retain or modify the buffer after the callback. Consider returning a copy or clearly documenting buffer ownership.
Suggested implementation:
```csharp
// Internal buffer for accumulating received data.
// Consumers should never retain or modify this buffer directly.
private readonly byte[] _data = new byte[length];
```
```csharp
private int _receivedLength;
```
```csharp
/// <summary>
/// Receives data and processes it as a fixed-length package.
/// The returned buffer to consumers is always a copy; consumers must not retain or modify the internal buffer.
/// </summary>
/// <param name="data">Incoming data chunk.</param>
/// <returns></returns>
public override async Task ReceiveAsync(Memory<byte> data)
{
// 处理上次粘包数据
data = ConcatBuffer(data);
```
If you have a callback or event that provides the completed data package to consumers (e.g., OnPackageReceived or similar), ensure you return a copy of the buffer, like:
<code>
var package = new byte[_receivedLength];
Array.Copy(_data, 0, package, 0, _receivedLength);
OnPackageReceived?.Invoke(package);
</code>
If this logic is not yet present, you should add it wherever the completed package is handed off to consumers.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:36` </location>
<code_context>
+ var segment = data.Length > len ? data[..len] : data;
+ segment.CopyTo(_data[_receivedLength..]);
+
+ if(data.Length > len)
+ {
+ HandlerStickyPackage(data, data.Length - len);
</code_context>
<issue_to_address>
HandlerStickyPackage may not handle multiple sticky packets in one buffer.
Currently, only the first complete packet is processed, and any additional packets are stored as sticky data. Please update the logic to process all complete packets in the buffer.
Suggested implementation:
```csharp
var offset = 0;
var remaining = data.Length;
while (remaining > 0)
{
var len = length - _receivedLength;
var take = remaining > len ? len : remaining;
var segment = data.Slice(offset, take);
segment.CopyTo(_data[_receivedLength..]);
_receivedLength += segment.Length;
offset += segment.Length;
remaining -= segment.Length;
if (_receivedLength == length)
{
// 触发回调
// (Assuming you have a method to handle a complete packet here)
// HandleCompletePacket(_data);
_receivedLength = 0;
}
else
{
// Not enough data for a complete packet, break and store the rest as sticky
break;
}
}
// If there is leftover data that doesn't complete a packet, store as sticky
if (remaining > 0)
{
HandlerStickyPackage(data.Slice(offset, remaining), remaining);
}
```
- You may need to ensure that the callback for a complete packet (e.g., `HandleCompletePacket(_data)`) is called at the correct place inside the loop.
- If `HandlerStickyPackage` expects the original buffer and offset, you may need to adjust its parameters accordingly.
- Remove or refactor any now-unnecessary code that handled sticky data outside the loop.
</issue_to_address>
### Comment 4
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:71` </location>
<code_context>
+ }
+
+ [Fact]
+ public async Task SendAsync_Error()
+ {
+ var client = CreateClient();
+
+ // 测试未建立连接前调用 SendAsync 方法报异常逻辑
+ var data = new Memory<byte>([1, 2, 3, 4, 5]);
+ var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => client.SendAsync(data));
+ Assert.Equal("TCP Socket is not connected 127.0.0.1:0", ex.Message);
+ }
+
</code_context>
<issue_to_address>
Assertion on exception message is brittle.
Instead of asserting the full exception message, assert the exception type and check that the message contains a relevant substring, or use a custom exception type for better test stability.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => client.SendAsync(data));
Assert.Equal("TCP Socket is not connected 127.0.0.1:0", ex.Message);
=======
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => client.SendAsync(data));
Assert.Contains("TCP Socket is not connected", ex.Message);
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:82` </location>
<code_context>
+ }
+
+ [Fact]
+ public async Task SendAsync_Cancel()
+ {
+ var port = 8881;
+ var server = StartTcpServer(port, MockSplitPackageAsync);
+
+ var client = CreateClient();
+ Assert.False(client.IsConnected);
+
+ // 连接 TCP Server
+ await client.ConnectAsync("localhost", port);
+ Assert.True(client.IsConnected);
+
+ // 测试 SendAsync 方法发送取消逻辑
+ var cst = new CancellationTokenSource();
+ cst.Cancel();
+
+ var data = new Memory<byte>([1, 2, 3, 4, 5]);
+ var result = await client.SendAsync(data, cst.Token);
+ Assert.False(result);
+
+ // 设置延时发送适配器
</code_context>
<issue_to_address>
Test for send cancellation is present.
Consider adding tests for partial sends or handling network interruptions during send operations to improve coverage.
</issue_to_address>
### Comment 6
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:126` </location>
<code_context>
+ public async Task ReceiveAsync_Error()
</code_context>
<issue_to_address>
Receive error handling is tested, but consider more edge cases.
Also test scenarios where the receive buffer is too small or the handler is null.
Suggested implementation:
```csharp
[Fact]
public async Task ReceiveAsync_Error()
{
var client = CreateClient();
// 测试未建立连接前调用 ReceiveAsync 方法报异常逻辑
var methodInfo = client.GetType().GetMethod("ReceiveAsync", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
Assert.NotNull(methodInfo);
var task = (Task)methodInfo.Invoke(client, null)!;
var ex = await Assert.ThrowsAsync<InvalidOperationException>(async () => await task);
Assert.NotNull(ex);
}
[Fact]
public async Task ReceiveAsync_BufferTooSmall_ThrowsException()
{
var client = CreateClient();
await client.ConnectAsync("127.0.0.1", 12345);
// 假设 ReceiveAsync 接受 buffer 参数
var methodInfo = client.GetType().GetMethod("ReceiveAsync", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
Assert.NotNull(methodInfo);
// 传递一个长度为0的 buffer
var buffer = new byte[0];
var task = (Task)methodInfo.Invoke(client, new object[] { buffer })!;
var ex = await Assert.ThrowsAsync<ArgumentException>(async () => await task);
Assert.NotNull(ex);
// 传递一个长度为1的 buffer（如果业务逻辑要求最小长度大于1）
buffer = new byte[1];
task = (Task)methodInfo.Invoke(client, new object[] { buffer })!;
// 这里假设长度为1也会抛异常，否则可以断言正常返回
// var ex2 = await Assert.ThrowsAsync<ArgumentException>(async () => await task);
// Assert.NotNull(ex2);
}
[Fact]
public async Task ReceiveAsync_NullHandler_ThrowsException()
{
var client = CreateClient();
await client.ConnectAsync("127.0.0.1", 12345);
// 假设 ReceiveAsync 接受 handler 参数
var methodInfo = client.GetType().GetMethod("ReceiveAsync", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
Assert.NotNull(methodInfo);
// 传递 null 作为 handler
object? handler = null;
var task = (Task)methodInfo.Invoke(client, new object[] { handler })!;
var ex = await Assert.ThrowsAsync<ArgumentNullException>(async () => await task);
Assert.NotNull(ex);
```
- If your `ReceiveAsync` method does not accept a buffer or handler parameter, you will need to adjust the reflection invocation and test logic to match your actual method signature.
- If the method is overloaded, ensure you select the correct overload via reflection.
- If the buffer or handler is set via a property or another method, adjust the test to set those before invoking `ReceiveAsync`.
- If the exception types differ (e.g., custom exceptions), replace `ArgumentException` and `ArgumentNullException` with the correct types.
</issue_to_address>
### Comment 7
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:57` </location>
<code_context>
+ /// for the specified <paramref name="length"/>.</remarks>
+ /// <param name="buffer">The memory buffer containing the data to process.</param>
+ /// <param name="length">The length of the valid data within the buffer.</param>
+ protected void HandlerStickyPackage(Memory<byte> buffer, int length)
+ {
+ var len = buffer.Length - length;
</code_context>
<issue_to_address>
Consider replacing MemoryPool usage with direct array allocations to simplify buffer management.
```csharp
// Instead of juggling MemoryPool<byte> and slicing MemoryBlocks, you can
// simplify to a plain array allocation (or List<byte>) with almost identical behavior:
private Memory<byte> _lastReceiveBuffer = Memory<byte>.Empty;
// …
protected void HandlerStickyPackage(Memory<byte> buffer, int length)
{
// if there’s overflow, just copy the tail into a new array
if (buffer.Length > length)
{
_lastReceiveBuffer = buffer[length..].ToArray();
}
}
protected Memory<byte> ConcatBuffer(Memory<byte> buffer)
{
if (_lastReceiveBuffer.IsEmpty)
{
return buffer;
}
// allocate exactly what we need, copy both pieces, clear state
var combined = new byte[_lastReceiveBuffer.Length + buffer.Length];
_lastReceiveBuffer.CopyTo(combined);
buffer.CopyTo(combined.AsMemory(_lastReceiveBuffer.Length));
_lastReceiveBuffer = Memory<byte>.Empty;
return combined;
}
```
This preserves your sticky‐packet behavior but:
1. Removes the MemoryPool<> rental/return logic
2. Eliminates the extra `IMemoryOwner<byte>` indirection
3. Keeps everything in simple arrays (or you could swap to a `List<byte>` if you prefer dynamic growth)
Functionally nothing changes, but the code is much easier to read and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:24` </location>
<code_context>
+
+ public bool IsConnected => _client?.Connected ?? false;
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
</code_context>
<issue_to_address>
LocalEndPoint should likely be read-only.
Making LocalEndPoint settable after construction may cause inconsistencies with the TcpClient binding. Recommend restricting it to be read-only and set only in the constructor.
Suggested implementation:
```csharp
public IPEndPoint LocalEndPoint { get; }
```
```csharp
public DefaultTcpSocketClient(string host, int port = 0)
{
LocalEndPoint = new IPEndPoint(GetIPAddress(host), port);
}
```
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked [NotNull].
Consider removing the nullable annotation or [NotNull] attribute to avoid this contradiction. If Logger is required, enforce its presence through constructor injection or null checks.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:162` </location>
<code_context>
+ Dispose(true);
+ }
+
+ private void Dispose(bool disposing)
+ {
+ if (disposing)
</code_context>
<issue_to_address>
Dispose pattern does not implement finalizer.
If there are no unmanaged resources, remove Dispose(bool) and only implement IDisposable.Dispose. If there are unmanaged resources, add a finalizer.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:17` </location>
<code_context>
+/// <param name="length">The data package total data length.</param>
+public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
+{
+ private readonly Memory<byte> _data = new byte[length];
+
+ private int _receivedLength;
</code_context>
<issue_to_address>
Using Memory<byte> for _data may not be necessary.
Consider using a byte[] for _data to simplify the code and improve efficiency, as the buffer size is fixed.
Suggested implementation:
```csharp
private readonly byte[] _data = new byte[length];
```
If there are any usages of `_data` as a `Memory<byte>` elsewhere in this class (e.g., passing `_data` to methods expecting `Memory<byte>`), you should update those usages to `_data.AsMemory()` or refactor as appropriate. Review the rest of the class for such cases.
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:36` </location>
<code_context>
+ var segment = data.Length > len ? data[..len] : data;
+ segment.CopyTo(_data[_receivedLength..]);
+
+ if(data.Length > len)
+ {
+ HandlerStickyPackage(data, data.Length - len);
</code_context>
<issue_to_address>
HandlerStickyPackage may be called with incorrect length.
Please verify that data.Length - len accurately represents the sticky data offset, especially when segment.Length differs from data.Length.
</issue_to_address>
### Comment 6
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:18` </location>
<code_context>
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
+
+ public ITcpSocketClient GetOrCreate(string host, int port = 0)
+ {
+ return _pool.GetOrAdd($"{host}:{port}", key =>
</code_context>
<issue_to_address>
Keying the pool by host:port string may cause collisions.
Consider using a tuple (host, port) or a dedicated struct as the dictionary key to avoid collisions and parsing issues, particularly with IPv6 addresses.
Suggested implementation:
```csharp
class DefaultTcpSocketFactory(IServiceProvider provider) : ITcpSocketFactory
{
private readonly struct HostPortKey : IEquatable<HostPortKey>
{
public readonly string Host;
public readonly int Port;
public HostPortKey(string host, int port)
{
Host = host;
Port = port;
}
public bool Equals(HostPortKey other) =>
string.Equals(Host, other.Host, StringComparison.Ordinal) && Port == other.Port;
public override bool Equals(object? obj) =>
obj is HostPortKey other && Equals(other);
public override int GetHashCode()
{
unchecked
{
return ((Host != null ? StringComparer.Ordinal.GetHashCode(Host) : 0) * 397) ^ Port;
}
}
}
private readonly ConcurrentDictionary<HostPortKey, ITcpSocketClient> _pool = new();
public ITcpSocketClient GetOrCreate(string host, int port = 0)
{
var key = new HostPortKey(host, port);
return _pool.GetOrAdd(key, _ =>
{
var client = new DefaultTcpSocketClient(host, port)
{
Logger = provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
}
public ITcpSocketClient? Remove(string host, int port)
```
You should also update the implementation of the Remove method to use the new HostPortKey struct as the key when removing from the dictionary.
</issue_to_address>
### Comment 7
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:37` </location>
<code_context>
+ var client3 = factory.GetOrCreate(ip.ToString(), 0);
+
+ // 测试不合格 IP 地址
+ var client4 = factory.GetOrCreate("256.0.0.1", 0);
+
+ var client5 = factory.Remove("256.0.0.1", 0);
</code_context>
<issue_to_address>
Test for invalid IP address is present but lacks assertion.
Please add an assertion to confirm the expected result when using an invalid IP address.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var client4 = factory.GetOrCreate("256.0.0.1", 0);
var client5 = factory.Remove("256.0.0.1", 0);
Assert.Equal(client4, client5);
Assert.NotNull(client5);
=======
var client4 = factory.GetOrCreate("256.0.0.1", 0);
Assert.NotNull(client4);
var client5 = factory.Remove("256.0.0.1", 0);
Assert.Equal(client4, client5);
Assert.NotNull(client5);
>>>>>>> REPLACE
</suggested_fix>
### Comment 8
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:34` </location>
<code_context>
+ Assert.Equal(client1, client2);
+
+ var ip = Dns.GetHostAddresses(Dns.GetHostName(), AddressFamily.InterNetwork).FirstOrDefault() ?? IPAddress.Loopback;
+ var client3 = factory.GetOrCreate(ip.ToString(), 0);
+
+ // 测试不合格 IP 地址
</code_context>
<issue_to_address>
No assertion for client3 creation with local IP.
Please add an assertion to verify that client3 is created correctly and behaves as expected.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var client3 = factory.GetOrCreate(ip.ToString(), 0);
// 测试不合格 IP 地址
=======
var client3 = factory.GetOrCreate(ip.ToString(), 0);
Assert.NotNull(client3);
Assert.NotEqual(client1, client3);
// 测试不合格 IP 地址
>>>>>>> REPLACE
</suggested_fix>
### Comment 9
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:131` </location>
<code_context>
+ var client = CreateClient();
+
+ // 测试未建立连接前调用 ReceiveAsync 方法报异常逻辑
+ var methodInfo = client.GetType().GetMethod("ReceiveAsync", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+ Assert.NotNull(methodInfo);
+
</code_context>
<issue_to_address>
Direct invocation of private ReceiveAsync method via reflection.
Accessing private methods via reflection in tests can lead to fragile code. Prefer exposing required behavior through the public API or internal access for testing. If reflection is necessary, document the reason.
Suggested implementation:
```csharp
var client = CreateClient();
// TODO: Refactor to test ReceiveAsync behavior via the public API or make ReceiveAsync internal for testing.
// Avoid using reflection to access private methods in tests as it leads to fragile code.
// Example: If ReceiveAsync is required for testing, consider exposing the necessary behavior through a public or internal method.
// For now, this test is skipped until the code is refactored for testability.
// throw new NotImplementedException("Test for ReceiveAsync should be implemented via public API or internal access.");
```
If you want to test `ReceiveAsync` directly, consider making it `internal` and adding `[assembly: InternalsVisibleTo("UnitTest")]` in your main project. Then, call it directly in your test without reflection.
</issue_to_address>
### Comment 10
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:232` </location>
<code_context>
+ var tcs = new TaskCompletionSource();
+ Memory<byte> receivedBuffer = Memory<byte>.Empty;
+
+ // 增加数据库处理适配器
+ client.SetDataHandler(new FixLengthDataPackageHandler(7)
+ {
</code_context>
<issue_to_address>
Typo in comment: '数据库' should be '数据包'.
Please update the comment to '增加数据包处理适配器' for accuracy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 增加数据库处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
=======
// 增加数据包处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
>>>>>>> REPLACE
</suggested_fix>
### Comment 11
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:253` </location>
<code_context>
+ // 验证接收到的数据
+ Assert.Equal(receivedBuffer.ToArray(), [1, 2, 3, 4, 5, 3, 4]);
+
+ // 等待第二次数据
+ receivedBuffer = Memory<byte>.Empty;
+ tcs = new TaskCompletionSource();
+ await tcs.Task;
+
+ // 验证第二次收到的数据
</code_context>
<issue_to_address>
Potential deadlock: second TaskCompletionSource is never set.
After resetting 'tcs', there is no code that sets its result, so 'await tcs.Task' may cause the test to hang. Please ensure the TaskCompletionSource is completed or add a timeout to avoid deadlocks.
</issue_to_address>
### Comment 12
<location> `test/UnitTest/Extensions/HostEnvironmentExtensionsTest.cs:17` </location>
<code_context>
+ public void IsWasm_Ok()
+ {
+ var hostEnvironment = new MockWasmHostEnvironment();
+ Assert.False(hostEnvironment.IsWasm());
+ }
+
</code_context>
<issue_to_address>
Test for IsWasm extension is present but only covers the negative case.
Please add a test to verify IsWasm returns true for a valid environment, or document why this isn't feasible.
Suggested implementation:
```csharp
[Fact]
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
[Fact]
public void IsWasm_ReturnsTrue_ForWasmEnvironment()
{
var wasmHostEnvironment = new MockWasmHostEnvironment();
wasmHostEnvironment.EnvironmentName = "Wasm"; // or whatever value triggers IsWasm() to return true
Assert.True(wasmHostEnvironment.IsWasm());
}
```
If `MockWasmHostEnvironment` does not have a public `EnvironmentName` property or another way to simulate a WASM environment, you will need to adjust the mock or create a new one that does. Make sure that the value you set matches the logic inside the `IsWasm()` extension method.
</issue_to_address>
### Comment 13
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:14` </location>
<code_context>
+namespace BootstrapBlazor.Components;
+
+[UnsupportedOSPlatform("browser")]
+class DefaultTcpSocketFactory(IServiceProvider provider) : ITcpSocketFactory
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
</code_context>
<issue_to_address>
Consider replacing the primary-constructor syntax with an explicit constructor and field for dependency injection to improve code clarity and familiarity.
You can remove the new “primary‐constructor” syntax and make the dependency injection explicit. This keeps the same behavior but is more familiar and reduces surprise:
```csharp
[UnsupportedOSPlatform("browser")]
class DefaultTcpSocketFactory : ITcpSocketFactory, IDisposable
{
private readonly IServiceProvider _provider;
private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
public DefaultTcpSocketFactory(IServiceProvider provider)
{
_provider = provider;
}
public ITcpSocketClient GetOrCreate(string host, int port = 0)
=> _pool.GetOrAdd($"{host}:{port}", key =>
{
var client = new DefaultTcpSocketClient(host, port)
{
Logger = _provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
public ITcpSocketClient? Remove(string host, int port)
{
return _pool.TryRemove($"{host}:{port}", out var c) ? c : null;
}
private void Dispose(bool disposing)
{
if (disposing)
{
foreach (var socket in _pool.Values)
{
socket.Dispose();
}
_pool.Clear();
}
}
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
}
```
Steps:
1. Change the class header to remove `(IServiceProvider provider)`.
2. Add a private `IServiceProvider _provider` field.
3. Introduce an explicit constructor that assigns `_provider`.
4. Update all uses of `provider` → `_provider`.
</issue_to_address>
### Comment 14
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:18` </location>
<code_context>
+/// implementation simply returns the provided data.</remarks>
+public abstract class DataPackageHandlerBase : IDataPackageHandler
+{
+ private Memory<byte> _lastReceiveBuffer = Memory<byte>.Empty;
+
+ /// <summary>
</code_context>
<issue_to_address>
Consider replacing the pooled memory buffer and two methods with a single List<byte> to manage leftover and concatenation logic.
Consider dropping the pooled‐`Memory<byte>` and two separate methods in favor of a simple `List<byte>` as your single growing buffer. You still get the same “store leftover + concat next chunk” behavior without pool lifetimes:
```csharp
// replace this field
// private Memory<byte> _lastReceiveBuffer = Memory<byte>.Empty;
private readonly List<byte> _buffer = new();
// simplified HandlerStickyPackage
protected void HandlerStickyPackage(ReadOnlyMemory<byte> buffer, int length)
{
var leftover = buffer.Slice(length);
if (!leftover.IsEmpty)
{
// store leftover in List<byte>
_buffer.AddRange(leftover.ToArray());
}
}
// simplified ConcatBuffer
protected ReadOnlyMemory<byte> ConcatBuffer(ReadOnlyMemory<byte> buffer)
{
if (_buffer.Count == 0)
{
return buffer;
}
// merge stored bytes + new buffer
_buffer.AddRange(buffer.ToArray());
var merged = _buffer.ToArray();
_buffer.Clear();
return merged;
}
```
Benefits:
- No pool‐rent/return bookkeeping
- Fewer moving parts: just one list, two methods
- All functionality (storing leftovers + merging) is preserved.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- DefaultTcpSocketFactory uses a primary-constructor parameter for IServiceProvider but never stores it—capture it in a private readonly field so
provider.GetServiceactually compiles and works. - HostEnvironmentExtensions.IsWasm is checking against a test-only MockWasmHostEnvironment type; replace that with a real WASM detection (e.g. via RuntimeInformation or OperatingSystem checks).
- DataPackageHandlerBase rents buffers from MemoryPool in ConcatBuffer/HandlerStickyPackage but never disposes the IMemoryOwner, which can leak—ensure rented blocks are released when no longer needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- DefaultTcpSocketFactory uses a primary-constructor parameter for IServiceProvider but never stores it—capture it in a private readonly field so `provider.GetService` actually compiles and works.
- HostEnvironmentExtensions.IsWasm is checking against a test-only MockWasmHostEnvironment type; replace that with a real WASM detection (e.g. via RuntimeInformation or OperatingSystem checks).
- DataPackageHandlerBase rents buffers from MemoryPool in ConcatBuffer/HandlerStickyPackage but never disposes the IMemoryOwner, which can leak—ensure rented blocks are released when no longer needed.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked with [NotNull], which is contradictory.
Consider removing the [NotNull] attribute or making Logger non-nullable and ensuring it is always initialized.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:67` </location>
<code_context>
+ await _client.ConnectAsync(endPoint, token);
+
+ // 开始接收数据
+ _ = Task.Run(ReceiveAsync, token);
+
+ LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
</code_context>
<issue_to_address>
Fire-and-forget ReceiveAsync task may hide exceptions.
Consider capturing the ReceiveAsync task and handling exceptions to avoid unobserved failures, or document this behavior if intentional.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:18` </location>
<code_context>
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
+
+ public ITcpSocketClient GetOrCreate(string host, int port = 0)
+ {
+ return _pool.GetOrAdd($"{host}:{port}", key =>
</code_context>
<issue_to_address>
Keying the pool by host:port string may cause issues with IPv6 addresses.
Consider using a tuple (host, port) as the dictionary key to prevent ambiguity and potential collisions with IPv6 addresses.
Suggested implementation:
```csharp
private readonly ConcurrentDictionary<(string host, int port), ITcpSocketClient> _pool = new();
public ITcpSocketClient GetOrCreate(string host, int port = 0)
{
return _pool.GetOrAdd((host, port), key =>
{
var client = new DefaultTcpSocketClient(key.host, key.port)
{
Logger = provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
}
public ITcpSocketClient? Remove(string host, int port)
```
You should also update the implementation of the `Remove` method to use the tuple key `(host, port)` when accessing `_pool`. For example:
```csharp
public ITcpSocketClient? Remove(string host, int port)
{
_pool.TryRemove((host, port), out var client);
return client;
}
```
</issue_to_address>
### Comment 4
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:37` </location>
<code_context>
+ var client3 = factory.GetOrCreate(ip.ToString(), 0);
+
+ // 测试不合格 IP 地址
+ var client4 = factory.GetOrCreate("256.0.0.1", 0);
+
+ var client5 = factory.Remove("256.0.0.1", 0);
</code_context>
<issue_to_address>
Missing assertion for invalid IP address handling.
Please add an assertion to verify the expected result when an invalid IP address is used.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var client4 = factory.GetOrCreate("256.0.0.1", 0);
var client5 = factory.Remove("256.0.0.1", 0);
Assert.Equal(client4, client5);
Assert.NotNull(client5);
=======
var client4 = factory.GetOrCreate("256.0.0.1", 0);
// Assert that the client returned for an invalid IP is not null (or adjust as per expected behavior)
Assert.NotNull(client4);
var client5 = factory.Remove("256.0.0.1", 0);
Assert.Equal(client4, client5);
Assert.NotNull(client5);
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:232` </location>
<code_context>
+ var tcs = new TaskCompletionSource();
+ Memory<byte> receivedBuffer = Memory<byte>.Empty;
+
+ // 增加数据库处理适配器
+ client.SetDataHandler(new FixLengthDataPackageHandler(7)
+ {
</code_context>
<issue_to_address>
Typo in comment: '数据库' should be '数据包'.
Please update the comment to '增加数据包处理适配器' for accuracy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 增加数据库处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
=======
// 增加数据包处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:253` </location>
<code_context>
+ // 验证接收到的数据
+ Assert.Equal(receivedBuffer.ToArray(), [1, 2, 3, 4, 5, 3, 4]);
+
+ // 等待第二次数据
+ receivedBuffer = Memory<byte>.Empty;
+ tcs = new TaskCompletionSource();
+ await tcs.Task;
+
+ // 验证第二次收到的数据
</code_context>
<issue_to_address>
Potential deadlock: second tcs.Task is never triggered.
After resetting receivedBuffer and tcs, tcs.SetResult() is never called, which may cause the test to hang. Please ensure the callback triggers tcs.SetResult() or add a timeout to avoid deadlocks.
</issue_to_address>
### Comment 7
<location> `test/UnitTest/Extensions/HostEnvironmentExtensionsTest.cs:17` </location>
<code_context>
+ public void IsWasm_Ok()
+ {
+ var hostEnvironment = new MockWasmHostEnvironment();
+ Assert.False(hostEnvironment.IsWasm());
+ }
+
</code_context>
<issue_to_address>
Test only covers the negative case for IsWasm.
Please add a positive test for IsWasm if feasible, or document why only the negative case is tested.
Suggested implementation:
```csharp
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
[Fact]
public void IsWasm_Positive_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
hostEnvironment.IsWasmValue = true; // Simulate WASM environment
Assert.True(hostEnvironment.IsWasm());
}
```
This assumes that `MockWasmHostEnvironment` has a public property or method (e.g., `IsWasmValue`) that allows you to control the return value of `IsWasm()`. If it does not, you will need to update the mock class to support this, for example by adding a settable property that `IsWasm()` returns.
</issue_to_address>
### Comment 8
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:15` </location>
<code_context>
+namespace BootstrapBlazor.Components;
+
+[UnsupportedOSPlatform("browser")]
+class DefaultTcpSocketClient : ITcpSocketClient
+{
+ private TcpClient? _client;
</code_context>
<issue_to_address>
Consider refactoring by extracting a try/catch logging helper, merging ConnectAsync overloads, and moving the receive loop to a dedicated method.
```markdown
I agree we can shrink the nesting and remove the boilerplate without losing any behavior. Here are three small focused steps:
1. Extract a generic “try/catch + log” helper
2. Collapse the two `ConnectAsync` overloads into one
3. Pull out your receive‐loop into a dedicated method
```csharp
// 1. Generic helper
private async Task<bool> TryAsync(
Func<Task> action,
string operation,
IPEndPoint? remote,
CancellationToken token = default)
{
try
{
await action();
return true;
}
catch (OperationCanceledException ex)
{
Logger.LogWarning(ex, "TCP {Operation} canceled from {Local} to {Remote}",
operation, LocalEndPoint, remote);
}
catch (Exception ex)
{
Logger.LogError(ex, "TCP {Operation} failed from {Local} to {Remote}",
operation, LocalEndPoint, remote);
}
return false;
}
// 2. Single ConnectAsync
public Task<bool> ConnectAsync(string host, int port, CancellationToken token = default)
{
var endPoint = new IPEndPoint(GetIPAddress(host), port);
return TryAsync(async () =>
{
Close();
_client = new TcpClient(LocalEndPoint);
await _client.ConnectAsync(endPoint, token);
LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
_remoteEndPoint = endPoint;
_ = Task.Run(() => ReceiveLoop(_receiveCts.Token), token);
},
operation: "Connect",
remote: endPoint,
token: token);
}
// 3. Dedicated receive loop
private async Task ReceiveLoop(CancellationToken token)
{
using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
var buffer = block.Memory;
while (!token.IsCancellationRequested && _client?.Connected == true)
{
try
{
var len = await _client.GetStream().ReadAsync(buffer, token);
if (len == 0) break;
var slice = buffer.Slice(0, len);
if (_dataPackageHandler != null)
await _dataPackageHandler.ReceiveAsync(slice);
}
catch (OperationCanceledException) { break; }
catch (Exception ex)
{
Logger.LogError(ex, "TCP Receive failed from {Local} to {Remote}", LocalEndPoint, _remoteEndPoint);
break;
}
}
}
```
This preserves exactly the same behavior, but:
- Eliminates duplicate/verbose try/catch blocks
- Merges the two `ConnectAsync` methods
- Moves the receive loop into its own method for clarity
```
</issue_to_address>
### Comment 9
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:18` </location>
<code_context>
+/// implementation simply returns the provided data.</remarks>
+public abstract class DataPackageHandlerBase : IDataPackageHandler
+{
+ private Memory<byte> _lastReceiveBuffer = Memory<byte>.Empty;
+
+ /// <summary>
</code_context>
<issue_to_address>
Consider replacing manual buffer management and pooling with a single MemoryStream for all sticky buffer handling.
```markdown
Consider replacing the two‐field + pool approach with a single `MemoryStream` that you append to and clear. This removes manual Rent/Return, cuts out `HandlerStickyPackage`, and brings all buffering into one place:
```csharp
public abstract class DataPackageHandlerBase : IDataPackageHandler
{
// replace _lastReceiveBuffer + pool usage
private readonly MemoryStream _stickyBuffer = new MemoryStream();
protected Memory<byte> ConcatBuffer(ReadOnlyMemory<byte> incoming)
{
if (incoming.IsEmpty)
return Memory<byte>.Empty;
// append incoming data
#if NET5_0_OR_GREATER
_stickyBuffer.Write(incoming.Span);
#else
var arr = incoming.ToArray();
_stickyBuffer.Write(arr, 0, arr.Length);
#endif
// expose the combined data without extra copy
var combined = new Memory<byte>(_stickyBuffer.GetBuffer(), 0, (int)_stickyBuffer.Length);
// reset for next round
_stickyBuffer.SetLength(0);
return combined;
}
// remove HandlerStickyPackage entirely
}
```
Steps to apply:
1. Delete `_lastReceiveBuffer` field and both `HandlerStickyPackage` & its callers.
2. Add a single `MemoryStream _stickyBuffer`.
3. Replace all calls to `ConcatBuffer(...)` & leftover handling with the above implementation.
This keeps semantics (you still accumulate partial packets and then emit one contiguous buffer), but removes manual pool management and a second state field.```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- DefaultTcpSocketFactory is declared with primary‐constructor syntax that C# does not support—please add an explicit constructor to inject IServiceProvider correctly.
- HostEnvironmentExtensions.IsWasm only checks for the test‐only MockWasmHostEnvironment type; switch to a real WASM detection (e.g. RuntimeInformation or EnvironmentName) instead of referencing a mock.
- In DefaultTcpSocketClient.ConnectAsync you use
_client ??= new TcpClient(...), which could reuse a closed instance—always create a fresh TcpClient after closing to avoid invalid state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- DefaultTcpSocketFactory is declared with primary‐constructor syntax that C# does not support—please add an explicit constructor to inject IServiceProvider correctly.
- HostEnvironmentExtensions.IsWasm only checks for the test‐only MockWasmHostEnvironment type; switch to a real WASM detection (e.g. RuntimeInformation or EnvironmentName) instead of referencing a mock.
- In DefaultTcpSocketClient.ConnectAsync you use `_client ??= new TcpClient(...)`, which could reuse a closed instance—always create a fresh TcpClient after closing to avoid invalid state.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked [NotNull]; consider enforcing initialization.
If Logger is essential, require it in the constructor or ensure it is always set before use to prevent null reference exceptions.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:67` </location>
<code_context>
+ await _client.ConnectAsync(endPoint, token);
+
+ // 开始接收数据
+ _ = Task.Run(ReceiveAsync, token);
+
+ LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
</code_context>
<issue_to_address>
Task.Run with token does not cancel the task creation.
To enable cancellation during ReceiveAsync execution, pass the token directly to ReceiveAsync instead of only to Task.Run.
Suggested implementation:
```csharp
// 开始接收数据
_ = Task.Run(() => ReceiveAsync(token), token);
```
If `ReceiveAsync` does not already accept a `CancellationToken` parameter, you must update its definition to include one, and ensure all internal logic respects the token for cancellation.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:45` </location>
<code_context>
+ _receivedLength += segment.Length;
+
+ // 如果已接收长度等于总长度则触发回调
+ if (_receivedLength == length)
+ {
+ // 重置已接收长度
</code_context>
<issue_to_address>
Potential data loss if more data arrives after expected length.
Currently, only the first complete package is processed if multiple arrive in one call. Please loop to handle all complete packages in the buffer.
</issue_to_address>
### Comment 4
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:232` </location>
<code_context>
+ var tcs = new TaskCompletionSource();
+ Memory<byte> receivedBuffer = Memory<byte>.Empty;
+
+ // 增加数据库处理适配器
+ client.SetDataHandler(new FixLengthDataPackageHandler(7)
+ {
</code_context>
<issue_to_address>
Typo in comment: '数据库' should be '数据包'.
The comment should read '增加数据包处理适配器' to accurately describe the addition of a data package handler.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 增加数据库处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
=======
// 增加数据包处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:113` </location>
<code_context>
+ return ret;
+ }
+
+ private async Task ReceiveAsync()
+ {
+ using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
</code_context>
<issue_to_address>
Consider extracting the receive loop and its error handling into helper methods to reduce nesting and improve readability.
```suggestion
The class currently mixes connection‐lifecycle, send/receive loops, buffer management, cancellation, logging and data‐package handling all in one place. You can reduce nesting and improve readability by extracting the receive loop (and its error handling) into a small helper. For example:
```csharp
// in DefaultTcpSocketClient
public async Task<bool> ConnectAsync(IPEndPoint endPoint, CancellationToken token = default)
{
// …existing connect logic…
_ = Task.Run(() => ReceiveLoopAsync(_client.GetStream(), _receiveCts.Token));
return ret;
}
private async Task ReceiveLoopAsync(NetworkStream stream, CancellationToken token)
{
using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
var buffer = block.Memory;
while (!token.IsCancellationRequested)
{
if (!TryReadOnce(stream, buffer, token, out var slice))
break;
if (_dataPackageHandler != null)
await _dataPackageHandler.ReceiveAsync(slice);
}
}
private bool TryReadOnce(NetworkStream stream,
Memory<byte> buffer,
CancellationToken token,
out ReadOnlyMemory<byte> slice)
{
slice = default;
try
{
var len = stream.ReadAsync(buffer, token).GetAwaiter().GetResult();
if (len == 0)
{
Logger.LogInformation(
"TCP Socket {LocalEndPoint} closed by {RemoteEndPoint}",
LocalEndPoint, _remoteEndPoint);
return false;
}
slice = buffer[..len];
return true;
}
catch (OperationCanceledException ex)
{
Logger.LogWarning(
ex, "Receive canceled {LocalEndPoint}→{RemoteEndPoint}",
LocalEndPoint, _remoteEndPoint);
return false;
}
catch (Exception ex)
{
Logger.LogError(
ex, "Receive failed {LocalEndPoint}→{RemoteEndPoint}",
LocalEndPoint, _remoteEndPoint);
return true; // continue loop
}
}
```
This moves the inner receive/body + try-catch into two small methods:
1. ReceiveLoopAsync – handles the pool, loop and cancellation.
2. TryReadOnce – handles one ReadAsync, buffer slice, and logging.
You can apply a similar pattern to `SendAsync` (extract a `TrySendOnce`) and it will make the class far flatter and easier to maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- The HostEnvironmentExtensions.IsWasm method currently only returns true for the internal MockWasmHostEnvironment type; update it to detect real WebAssembly environments instead of relying on a test stub.
- Avoid using C# 12 primary‐constructor class syntax (e.g.
class DefaultTcpSocketFactory(IServiceProvider provider)) for broader compiler compatibility—define an explicit constructor and store the provider in a private field instead. - Consider disposing the ITcpSocketClient instance inside Remove(...) so that removed sockets don’t leak resources when they’re no longer needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The HostEnvironmentExtensions.IsWasm method currently only returns true for the internal MockWasmHostEnvironment type; update it to detect real WebAssembly environments instead of relying on a test stub.
- Avoid using C# 12 primary‐constructor class syntax (e.g. `class DefaultTcpSocketFactory(IServiceProvider provider)`) for broader compiler compatibility—define an explicit constructor and store the provider in a private field instead.
- Consider disposing the ITcpSocketClient instance inside Remove(...) so that removed sockets don’t leak resources when they’re no longer needed.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked [NotNull], which is contradictory.
Consider removing [NotNull] or making Logger non-nullable and ensuring it is always initialized to avoid confusion and potential issues with static analysis.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:86` </location>
<code_context>
+
+ public async Task<bool> SendAsync(Memory<byte> data, CancellationToken token = default)
+ {
+ if (_client is not { Connected: true })
+ {
+ throw new InvalidOperationException($"TCP Socket is not connected {LocalEndPoint}");
</code_context>
<issue_to_address>
Throwing InvalidOperationException may not be ideal for async SendAsync.
Consider returning false or a failed Task instead of throwing, so consumers aren't required to handle common states with exceptions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (_client is not { Connected: true })
{
throw new InvalidOperationException($"TCP Socket is not connected {LocalEndPoint}");
}
=======
if (_client is not { Connected: true })
{
return false;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:81` </location>
<code_context>
+
+ // 计算缓存区长度
+ var total = _lastReceiveBuffer.Length + buffer.Length;
+ var merged = new byte[total];
+ _lastReceiveBuffer.CopyTo(merged);
+ buffer.CopyTo(merged.AsMemory(_lastReceiveBuffer.Length));
</code_context>
<issue_to_address>
Allocating a new array for every buffer concat may impact performance.
Consider using ArrayPool<byte> for buffer allocations to minimize GC pressure if concatenations are large or frequent.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 计算缓存区长度
var total = _lastReceiveBuffer.Length + buffer.Length;
var merged = new byte[total];
_lastReceiveBuffer.CopyTo(merged);
buffer.CopyTo(merged.AsMemory(_lastReceiveBuffer.Length));
// Clear the sticky buffer
_lastReceiveBuffer = Memory<byte>.Empty;
return merged;
=======
// 计算缓存区长度
var total = _lastReceiveBuffer.Length + buffer.Length;
var pool = System.Buffers.ArrayPool<byte>.Shared;
var merged = pool.Rent(total);
try
{
_lastReceiveBuffer.CopyTo(merged);
buffer.CopyTo(new Memory<byte>(merged, _lastReceiveBuffer.Length, buffer.Length));
// Clear the sticky buffer
_lastReceiveBuffer = Memory<byte>.Empty;
// Return a copy to avoid holding onto the pooled array if the caller doesn't return it
var result = new byte[total];
Buffer.BlockCopy(merged, 0, result, 0, total);
return result;
}
finally
{
pool.Return(merged);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:18` </location>
<code_context>
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
+
+ public ITcpSocketClient GetOrCreate(string host, int port = 0)
+ {
+ return _pool.GetOrAdd($"{host}:{port}", key =>
</code_context>
<issue_to_address>
Keying the pool by string may cause issues with IPv6 or host aliases.
Consider normalizing the host or using a (host, port) tuple as the key to avoid collisions, especially with IPv6 or host aliases.
Suggested implementation:
```csharp
private readonly ConcurrentDictionary<(string host, int port), ITcpSocketClient> _pool = new();
public ITcpSocketClient GetOrCreate(string host, int port = 0)
{
return _pool.GetOrAdd((host, port), key =>
{
var client = new DefaultTcpSocketClient(key.host, key.port)
{
Logger = provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
}
public ITcpSocketClient? Remove(string host, int port)
```
```csharp
public ITcpSocketClient? Remove(string host, int port)
```
You should also update the implementation of the `Remove` method to use the tuple key `(host, port)` when removing from `_pool`:
```csharp
public ITcpSocketClient? Remove(string host, int port)
{
_pool.TryRemove((host, port), out var client);
return client;
}
```
</issue_to_address>
### Comment 5
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:36` </location>
<code_context>
+ var ip = Dns.GetHostAddresses(Dns.GetHostName(), AddressFamily.InterNetwork).FirstOrDefault() ?? IPAddress.Loopback;
+ var client3 = factory.GetOrCreate(ip.ToString(), 0);
+
+ // 测试不合格 IP 地址
+ var client4 = factory.GetOrCreate("256.0.0.1", 0);
+
+ var client5 = factory.Remove("256.0.0.1", 0);
</code_context>
<issue_to_address>
Consider asserting the behavior for invalid IP addresses.
Please add assertions to confirm the expected result when an invalid IP is used, such as an exception, fallback, or error log.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 测试不合格 IP 地址
var client4 = factory.GetOrCreate("256.0.0.1", 0);
var client5 = factory.Remove("256.0.0.1", 0);
Assert.Equal(client4, client5);
Assert.NotNull(client5);
client5.Dispose();
=======
// 测试不合格 IP 地址
// Assert the behavior for invalid IP addresses
var invalidIp = "256.0.0.1";
var client4 = factory.GetOrCreate(invalidIp, 0);
// Assert that the client is not null (or adjust if an exception is expected)
Assert.NotNull(client4);
// Optionally, assert that the client is a fallback or has a specific state
// For example, if the client should be a loopback or a mock, assert accordingly
// Assert.IsType<ExpectedType>(client4);
var client5 = factory.Remove(invalidIp, 0);
Assert.Equal(client4, client5);
Assert.NotNull(client5);
// Optionally, check for error logs if logging is testable
client5.Dispose();
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:113` </location>
<code_context>
+ return ret;
+ }
+
+ private async Task ReceiveAsync()
+ {
+ using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
</code_context>
<issue_to_address>
Consider simplifying the receive loop by using a single reusable byte array buffer, passing the cancellation token directly, and moving stream initialization outside the loop.
```suggestion
The core receive loop can be greatly simplified by:
1. Dropping `MemoryPool` and slicing logic in favor of a single reusable `byte[]` buffer.
2. Passing a CancellationToken directly into your loop instead of creating a separate `CancellationTokenSource`.
3. Hoisting `NetworkStream` outside of the loop.
For example, replace your current `ReceiveAsync` and the `Task.Run` in `ConnectAsync` with:
```csharp
public async Task<bool> ConnectAsync(IPEndPoint endPoint, CancellationToken token = default)
{
Close();
_client = new TcpClient(LocalEndPoint);
await _client.ConnectAsync(endPoint, token);
LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
_remoteEndPoint = endPoint;
// Kick off read-loop using the same token
_ = Task.Run(() => ReadLoopAsync(token), token);
return true;
}
private async Task ReadLoopAsync(CancellationToken token)
{
var stream = _client!.GetStream();
var buffer = new byte[ReceiveBufferSize];
try
{
while (!token.IsCancellationRequested)
{
int bytesRead = await stream.ReadAsync(buffer, 0, buffer.Length, token);
if (bytesRead == 0) break; // remote closed
// Pass slice to handler
if (_dataPackageHandler != null)
await _dataPackageHandler.ReceiveAsync(buffer.AsMemory(0, bytesRead));
}
}
catch (OperationCanceledException) { /* log warning if needed */ }
catch (Exception ex)
{
Logger.LogError(ex, "TCP Socket receive failed from {Local} to {Remote}", LocalEndPoint, _remoteEndPoint);
}
}
```
Benefits:
- Eliminates manual pooling/slicing.
- Reuses one buffer.
- Centralizes cancellation logic.
- Retains all existing logging and handler calls.
```
</issue_to_address>
### Comment 7
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:14` </location>
<code_context>
+namespace BootstrapBlazor.Components;
+
+[UnsupportedOSPlatform("browser")]
+class DefaultTcpSocketFactory(IServiceProvider provider) : ITcpSocketFactory
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
</code_context>
<issue_to_address>
Consider injecting ILogger directly, simplifying Remove to a one-liner, and collapsing Dispose overloads for clarity and conciseness.
```csharp
// Instead of capturing IServiceProvider, inject the ILogger directly;
// simplify Remove to a one‐liner and collapse Dispose overloads.
class DefaultTcpSocketFactory : ITcpSocketFactory, IDisposable
{
private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
private readonly ILogger<DefaultTcpSocketClient> _logger;
public DefaultTcpSocketFactory(ILogger<DefaultTcpSocketClient> logger)
{
_logger = logger;
}
public ITcpSocketClient GetOrCreate(string host, int port = 0)
=> _pool.GetOrAdd(
$"{host}:{port}",
_ => new DefaultTcpSocketClient(host, port) { Logger = _logger }
);
public ITcpSocketClient? Remove(string host, int port)
=> _pool.TryRemove($"{host}:{port}", out var client) ? client : null;
public void Dispose()
{
foreach (var socket in _pool.Values)
socket.Dispose();
_pool.Clear();
}
}
```
Actionable steps:
1. Remove the primary‐constructor syntax and the `IServiceProvider` field.
2. Inject `ILogger<DefaultTcpSocketClient>` in the ctor, assign to `_logger`.
3. Inline `Remove` with the ternary form.
4. Collapse `Dispose(bool)` into a single `Dispose`—no unmanaged resources require a full pattern.
</issue_to_address>
### Comment 8
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:26` </location>
<code_context>
+ /// </summary>
+ /// <param name="data"></param>
+ /// <returns></returns>
+ public override async Task ReceiveAsync(Memory<byte> data)
+ {
+ // 处理上次粘包数据
</code_context>
<issue_to_address>
Consider replacing the current receive logic with a single loop that processes arbitrary-sized chunks and eliminates extra buffer handling.
```csharp
public override async Task ReceiveAsync(Memory<byte> data)
{
int offset = 0;
while (offset < data.Length)
{
// how many bytes we still need
int remaining = length - _receivedLength;
// how many bytes are available in this chunk
int available = data.Length - offset;
int toCopy = Math.Min(remaining, available);
// copy into our fixed‐length buffer
data.Slice(offset, toCopy)
.CopyTo(_data.Slice(_receivedLength, toCopy));
_receivedLength += toCopy;
offset += toCopy;
// if we've filled a package, invoke the callback and reset
if (_receivedLength == length)
{
_receivedLength = 0;
if (ReceivedCallBack != null)
{
await ReceivedCallBack(_data);
}
}
}
}
```
This removes the `ConcatBuffer`/`HandlerStickyPackage` round‐trip, keeps the same semantics, and reads arbitrary‐sized chunks in a single loop.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- DefaultTcpSocketFactory is declared with a primary constructor (
class DefaultTcpSocketFactory(IServiceProvider provider)) but never stores the provider, which will break DI and logging—convert it to a standard constructor and keep the provider in a private field. - In DefaultTcpSocketClient.ConnectAsync you use
_client ??= new TcpClienteven after calling Close(), which may reuse a closed socket—always instantiate a fresh TcpClient after disposing the old one. - HostEnvironmentExtensions.IsWasm simply checks for a
MockWasmHostEnvironmenttype rather than detecting a real WASM runtime; consider using a runtime API (e.g.OperatingSystem.IsBrowser()) or feature detection instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- DefaultTcpSocketFactory is declared with a primary constructor (`class DefaultTcpSocketFactory(IServiceProvider provider)`) but never stores the provider, which will break DI and logging—convert it to a standard constructor and keep the provider in a private field.
- In DefaultTcpSocketClient.ConnectAsync you use `_client ??= new TcpClient` even after calling Close(), which may reuse a closed socket—always instantiate a fresh TcpClient after disposing the old one.
- HostEnvironmentExtensions.IsWasm simply checks for a `MockWasmHostEnvironment` type rather than detecting a real WASM runtime; consider using a runtime API (e.g. `OperatingSystem.IsBrowser()`) or feature detection instead.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:24` </location>
<code_context>
+
+ public bool IsConnected => _client?.Connected ?? false;
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
</code_context>
<issue_to_address>
LocalEndPoint should likely be read-only.
Making LocalEndPoint settable after construction may cause inconsistencies, as TcpClient relies on this value. Recommend restricting it to be set only in the constructor.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
public IPEndPoint LocalEndPoint { get; set; }
=======
public IPEndPoint LocalEndPoint { get; }
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked [NotNull].
If Logger must not be null, make it non-nullable and require it in the constructor. Otherwise, remove the [NotNull] attribute to avoid confusion.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:115` </location>
<code_context>
+
+ private async Task ReceiveAsync()
+ {
+ using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
+ var buffer = block.Memory;
+ _receiveCancellationTokenSource ??= new();
</code_context>
<issue_to_address>
Reusing buffer variable may cause issues with pooled memory.
Assign the sliced buffer to a new variable to prevent accidental misuse of the pooled memory and avoid potential bugs.
Suggested implementation:
```csharp
private async Task ReceiveAsync()
{
using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
var buffer = block.Memory;
var receiveBuffer = buffer.Slice(0, ReceiveBufferSize);
_receiveCancellationTokenSource ??= new();
while (_receiveCancellationTokenSource is { IsCancellationRequested: false })
{
```
- If the rest of the method uses `buffer` for receiving data, update those usages to use `receiveBuffer` instead.
- If you need to slice the buffer further for any reason, always assign the result to a new variable rather than reusing `buffer` or `receiveBuffer`.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:162` </location>
<code_context>
+ Dispose(true);
+ }
+
+ private void Dispose(bool disposing)
+ {
+ if (disposing)
</code_context>
<issue_to_address>
Dispose pattern does not implement finalizer.
If unmanaged resources aren't used, you can remove the Dispose(bool) overload. Otherwise, add a finalizer to support proper cleanup.
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:81` </location>
<code_context>
+
+ // 计算缓存区长度
+ var total = _lastReceiveBuffer.Length + buffer.Length;
+ var merged = new byte[total];
+ _lastReceiveBuffer.CopyTo(merged);
+ buffer.CopyTo(merged.AsMemory(_lastReceiveBuffer.Length));
</code_context>
<issue_to_address>
Allocating new array for every buffer concat may impact performance.
Consider using ArrayPool<byte> to reuse buffers and minimize allocations, which can help reduce GC pressure in high-frequency scenarios.
Suggested implementation:
```csharp
// 计算缓存区长度
var total = _lastReceiveBuffer.Length + buffer.Length;
var merged = System.Buffers.ArrayPool<byte>.Shared.Rent(total);
_lastReceiveBuffer.CopyTo(merged);
buffer.CopyTo(merged.AsMemory(_lastReceiveBuffer.Length));
// Clear the sticky buffer
_lastReceiveBuffer = Memory<byte>.Empty;
// Note: The returned Memory<byte> is backed by a pooled array.
// The caller is responsible for returning the array to the pool when done:
// System.Buffers.ArrayPool<byte>.Shared.Return(merged);
return new Memory<byte>(merged, 0, total);
```
If the caller is not already returning the pooled array to the pool, you must ensure that the buffer is returned to the pool after use to avoid memory leaks.
If you want to avoid leaking the implementation detail, consider wrapping the pooled array in a custom struct or class that implements IDisposable and returns the array to the pool on Dispose.
</issue_to_address>
### Comment 6
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:16` </location>
<code_context>
+[UnsupportedOSPlatform("browser")]
+class DefaultTcpSocketFactory(IServiceProvider provider) : ITcpSocketFactory
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
+
+ public ITcpSocketClient GetOrCreate(string host, int port = 0)
</code_context>
<issue_to_address>
String key for host:port may cause collisions with IPv6.
Consider using a tuple (host, port) or another structure for the key to prevent ambiguity with IPv6 addresses, which may contain colons.
Suggested implementation:
```csharp
private readonly ConcurrentDictionary<(string host, int port), ITcpSocketClient> _pool = new();
```
```csharp
return _pool.GetOrAdd((host, port), key =>
{
var client = new DefaultTcpSocketClient(key.host, key.port)
{
Logger = provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
```
</issue_to_address>
### Comment 7
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:232` </location>
<code_context>
+ var tcs = new TaskCompletionSource();
+ Memory<byte> receivedBuffer = Memory<byte>.Empty;
+
+ // 增加数据库处理适配器
+ client.SetDataHandler(new FixLengthDataPackageHandler(7)
+ {
</code_context>
<issue_to_address>
Typo in comment: '数据库' should be '数据包'.
The comment should read '增加数据包处理适配器' to accurately describe the addition of a data package handler.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 增加数据库处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
=======
// 增加数据包处理适配器
client.SetDataHandler(new FixLengthDataPackageHandler(7)
{
>>>>>>> REPLACE
</suggested_fix>
### Comment 8
<location> `test/UnitTest/Extensions/HostEnvironmentExtensionsTest.cs:13` </location>
<code_context>
+
+public class HostEnvironmentExtensionsTest
+{
+ [Fact]
+ public void IsWasm_Ok()
+ {
</code_context>
<issue_to_address>
Test for IsWasm covers only the negative case.
Consider adding a positive test case or a comment explaining why testing in a real WASM environment isn't feasible, to clarify expected behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
[Fact]
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
=======
// Only the negative case is tested here because running tests in a real WASM environment is not feasible
// in the current test infrastructure. If WASM testing becomes available, add a positive test case.
[Fact]
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
// [Fact(Skip = "Cannot run in WASM environment in current test infrastructure")]
// public void IsWasm_PositiveCase()
// {
// // This test would require running in a real WASM environment.
// // Assert.True(hostEnvironment.IsWasm());
// }
>>>>>>> REPLACE
</suggested_fix>
### Comment 9
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:16` </location>
<code_context>
+/// implementation simply returns the provided data.</remarks>
+public abstract class DataPackageHandlerBase : IDataPackageHandler
+{
+ private Memory<byte> _lastReceiveBuffer = Memory<byte>.Empty;
+
+ /// <summary>
</code_context>
<issue_to_address>
Consider replacing manual buffer management with SequenceReader<T> and a sequence field to streamline data handling and frame extraction.
```csharp
// Replace the manual “sticky‐packet” fields and methods with SequenceReader<T> to simplify buffering
using System.Buffers;
using System.Buffers.SequenceReader;
// remove: Memory<byte> _lastReceiveBuffer, HandlerStickyPackage and ConcatBuffer
private Sequence<byte> _receiveBuffer;
public override async Task ReceiveAsync(Memory<byte> data)
{
// append new data
_receiveBuffer.Write(data.Span);
// use SequenceReader to slice out complete frames
var reader = new SequenceReader<byte>(_receiveBuffer);
while (TryReadFrame(ref reader, out ReadOnlySequence<byte> frame))
{
// fire callback
if (ReceivedCallBack != null)
await ReceivedCallBack(frame.ToArray().AsMemory());
}
// retain unread (partial) data
_receiveBuffer = new Sequence<byte>(reader.UnreadSequence);
}
// Derived classes implement frame detection
protected abstract bool TryReadFrame(
ref SequenceReader<byte> reader,
out ReadOnlySequence<byte> frame);
```
Steps:
1. Remove `_lastReceiveBuffer`, `HandlerStickyPackage`, and `ConcatBuffer`.
2. Add a `Sequence<byte>` field.
3. In `ReceiveAsync`, write incoming data into the sequence, then loop with `SequenceReader<byte>` to parse full frames.
4. Store any leftover in the sequence’s `UnreadSequence`.
5. Let subclasses implement `TryReadFrame` to extract packet boundaries.
</issue_to_address>
### Comment 10
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:26` </location>
<code_context>
+ /// </summary>
+ /// <param name="data"></param>
+ /// <returns></returns>
+ public override async Task ReceiveAsync(Memory<byte> data)
+ {
+ // 处理上次粘包数据
</code_context>
<issue_to_address>
Consider simplifying the ReceiveAsync method by using a while loop with Span slicing to process incoming data and removing unnecessary buffer concatenation logic.
```csharp
public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
{
private readonly Memory<byte> _data = new byte[length];
private int _receivedLength;
public override async Task ReceiveAsync(Memory<byte> buffer)
{
// Process the incoming buffer until it's fully consumed:
var span = buffer.Span;
while (span.Length > 0)
{
int remaining = length - _receivedLength;
int toCopy = Math.Min(remaining, span.Length);
// Copy exactly what we need this round
span.Slice(0, toCopy).CopyTo(_data.Span.Slice(_receivedLength));
_receivedLength += toCopy;
// Advance the slice
span = span.Slice(toCopy);
// If we've got a full package, invoke the callback and reset
if (_receivedLength == length)
{
_receivedLength = 0;
if (ReceivedCallBack != null)
{
await ReceivedCallBack(_data);
}
}
}
}
}
```
Actionable changes:
1. Drop `ConcatBuffer(...)` and `HandlerStickyPackage(...)` entirely.
2. Use a simple `while` loop + `Span` slicing to “read exactly N bytes” directly.
3. Preserve the same reset/callback behavior when `_receivedLength == length`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Show resolved
Hide resolved
This reverts commit 44e4bd3.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- The use of primary‐constructor syntax in DefaultTcpSocketFactory and FixLengthDataPackageHandler isn’t valid C#—please define explicit constructors and backing fields so IServiceProvider and the
lengthparameter are properly captured. - HostEnvironmentExtensions.IsWasm currently checks for MockWasmHostEnvironment (a test class) rather than detecting WebAssembly at runtime; please replace it with a real runtime check (e.g. OperatingSystem.IsBrowser or similar).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The use of primary‐constructor syntax in DefaultTcpSocketFactory and FixLengthDataPackageHandler isn’t valid C#—please define explicit constructors and backing fields so IServiceProvider and the `length` parameter are properly captured.
- HostEnvironmentExtensions.IsWasm currently checks for MockWasmHostEnvironment (a test class) rather than detecting WebAssembly at runtime; please replace it with a real runtime check (e.g. OperatingSystem.IsBrowser or similar).
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked as [NotNull], which is contradictory.
Consider removing the [NotNull] attribute or making Logger non-nullable and ensuring it is always set to avoid confusion.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:67` </location>
<code_context>
+ await _client.ConnectAsync(endPoint, token);
+
+ // 开始接收数据
+ _ = Task.Run(ReceiveAsync, token);
+
+ LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
</code_context>
<issue_to_address>
Fire-and-forget ReceiveAsync may hide exceptions.
Consider capturing the ReceiveAsync task and handling or logging exceptions to prevent silent failures from unobserved exceptions.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:55` </location>
<code_context>
+ /// for the specified <paramref name="length"/>.</remarks>
+ /// <param name="buffer">The memory buffer containing the data to process.</param>
+ /// <param name="length">The length of the valid data within the buffer.</param>
+ protected void HandlerStickyPackage(Memory<byte> buffer, int length)
+ {
+ if (buffer.Length > length)
</code_context>
<issue_to_address>
Method name 'HandlerStickyPackage' may be a typo.
Consider renaming to 'HandleStickyPackage' for clarity and consistency with standard naming conventions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Handles the processing of a sticky package by adjusting the provided buffer and length.
/// </summary>
/// <remarks>This method processes the portion of the buffer beyond the specified length and updates the
/// internal state accordingly. The caller must ensure that the <paramref name="buffer"/> contains sufficient data
/// for the specified <paramref name="length"/>.</remarks>
/// <param name="buffer">The memory buffer containing the data to process.</param>
/// <param name="length">The length of the valid data within the buffer.</param>
protected void HandlerStickyPackage(Memory<byte> buffer, int length)
{
if (buffer.Length > length)
{
_lastReceiveBuffer = buffer[length..].ToArray().AsMemory();
}
}
=======
/// Handles the processing of a sticky package by adjusting the provided buffer and length.
/// </summary>
/// <remarks>This method processes the portion of the buffer beyond the specified length and updates the
/// internal state accordingly. The caller must ensure that the <paramref name="buffer"/> contains sufficient data
/// for the specified <paramref name="length"/>.</remarks>
/// <param name="buffer">The memory buffer containing the data to process.</param>
/// <param name="length">The length of the valid data within the buffer.</param>
protected void HandleStickyPackage(Memory<byte> buffer, int length)
{
if (buffer.Length > length)
{
_lastReceiveBuffer = buffer[length..].ToArray().AsMemory();
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked [NotNull], which is contradictory.
Please remove the [NotNull] attribute or make Logger non-nullable and ensure it is always set to avoid confusion about its nullability.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:63` </location>
<code_context>
+ Close();
+
+ // 创建新的 TcpClient 实例
+ _client ??= new TcpClient(LocalEndPoint);
+ await _client.ConnectAsync(endPoint, token);
+
</code_context>
<issue_to_address>
TcpClient is only created if null, but Close() sets it to null each time.
The null-coalescing assignment is unnecessary; assign a new TcpClient directly or clarify _client's lifecycle.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
// 创建新的 TcpClient 实例
_client ??= new TcpClient(LocalEndPoint);
await _client.ConnectAsync(endPoint, token);
=======
// 创建新的 TcpClient 实例
_client = new TcpClient(LocalEndPoint);
await _client.ConnectAsync(endPoint, token);
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:161` </location>
<code_context>
+ Dispose(true);
+ }
+
+ private void Dispose(bool disposing)
+ {
+ if (disposing)
+ {
+ _remoteEndPoint = null;
+
+ // 取消接收数据的任务
+ if (_receiveCancellationTokenSource is not null)
+ {
+ _receiveCancellationTokenSource.Cancel();
+ _receiveCancellationTokenSource.Dispose();
+ _receiveCancellationTokenSource = null;
+ }
+
+ // 释放 TcpClient 资源
</code_context>
<issue_to_address>
Dispose pattern does not implement finalizer, but calls GC.SuppressFinalize.
You can remove GC.SuppressFinalize since there's no finalizer implemented.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs:72` </location>
<code_context>
+ /// <param name="buffer">The buffer to concatenate with the previously stored data. Must not be empty.</param>
+ /// <returns>A <see cref="Memory{T}"/> instance containing the concatenated data. If no previously stored data exists, the
+ /// method returns the input <paramref name="buffer"/>.</returns>
+ protected Memory<byte> ConcatBuffer(Memory<byte> buffer)
+ {
+ if (_lastReceiveBuffer.IsEmpty)
</code_context>
<issue_to_address>
ConcatBuffer allocates a new array for every call with sticky data.
Frequent allocation of new arrays when _lastReceiveBuffer is not empty may cause memory pressure. Consider implementing array reuse or a buffer pool.
Suggested implementation:
```csharp
// 计算缓存区长度
var total = _lastReceiveBuffer.Length + buffer.Length;
var merged = System.Buffers.ArrayPool<byte>.Shared.Rent(total);
_lastReceiveBuffer.CopyTo(merged);
buffer.CopyTo(merged.AsMemory(_lastReceiveBuffer.Length));
```
```csharp
var result = new Memory<byte>(merged, 0, total);
// Optionally clear _lastReceiveBuffer here if needed
// _lastReceiveBuffer = Memory<byte>.Empty;
// Note: The caller is responsible for returning the array to the pool if needed.
// You may want to provide a method to release the buffer, or use IMemoryOwner<byte> for safer pooling.
return result;
```
- You should ensure that the rented array is returned to the pool when it is no longer needed. This may require additional changes to your class to track and release the buffer, or to use `IMemoryOwner<byte>` from `System.Buffers` for safer memory management.
- Update documentation/comments to warn consumers not to hold onto the returned memory after the next call or after the buffer is released.
- If the consumer of `ConcatBuffer` cannot be trusted to return the array, consider refactoring the API to return an `IMemoryOwner<byte>` instead of `Memory<byte>`.
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:18` </location>
<code_context>
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
+
+ public ITcpSocketClient GetOrCreate(string host, int port = 0)
+ {
+ return _pool.GetOrAdd($"{host}:{port}", key =>
</code_context>
<issue_to_address>
Keying the pool by string interpolation may cause issues with IPv6 or host normalization.
Consider normalizing the host or using a tuple (host, port) as the key to avoid potential collisions or mismatches from different host formats.
Suggested implementation:
```csharp
private readonly ConcurrentDictionary<(string host, int port), ITcpSocketClient> _pool = new();
public ITcpSocketClient GetOrCreate(string host, int port = 0)
{
return _pool.GetOrAdd((host, port), key =>
{
var client = new DefaultTcpSocketClient(key.host, key.port)
{
Logger = provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
}
public ITcpSocketClient? Remove(string host, int port)
```
You should also update the implementation of the `Remove` method to use the tuple key `(host, port)` when removing from `_pool`, for consistency and correctness.
</issue_to_address>
### Comment 6
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:253` </location>
<code_context>
+ // 验证接收到的数据
+ Assert.Equal(receivedBuffer.ToArray(), [1, 2, 3, 4, 5, 3, 4]);
+
+ // 等待第二次数据
+ receivedBuffer = Memory<byte>.Empty;
+ tcs = new TaskCompletionSource();
+ await tcs.Task;
+
+ // 验证第二次收到的数据
</code_context>
<issue_to_address>
Potential deadlock: second TaskCompletionSource is never triggered.
After resetting receivedBuffer and tcs, tcs.SetResult() is not called for the second data reception, which may cause the test to hang. Please ensure the callback is triggered or add a timeout to avoid deadlocks.
</issue_to_address>
### Comment 7
<location> `test/UnitTest/Extensions/HostEnvironmentExtensionsTest.cs:13` </location>
<code_context>
+
+public class HostEnvironmentExtensionsTest
+{
+ [Fact]
+ public void IsWasm_Ok()
+ {
+ var hostEnvironment = new MockWasmHostEnvironment();
+ Assert.False(hostEnvironment.IsWasm());
+ }
+
</code_context>
<issue_to_address>
Test only covers the negative case for IsWasm.
Please add a positive test case for IsWasm returning true, or document why this isn't possible.
Suggested implementation:
```csharp
[Fact]
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
[Fact]
public void IsWasm_ReturnsTrue_WhenWasmEnvironment()
{
// Arrange: Use a mock or stub that simulates a WASM environment
var wasmHostEnvironment = new MockWasmHostEnvironment(isWasm: true);
// Act & Assert
Assert.True(wasmHostEnvironment.IsWasm());
}
```
If `MockWasmHostEnvironment` does not currently support simulating a WASM environment (e.g., via a constructor parameter or property), you will need to update its implementation to allow this. If this is not possible, instead add a comment above the test explaining why a positive test cannot be written.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataPackage/DataPackageHandlerBase.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:24` </location>
<code_context>
+
+ public bool IsConnected => _client?.Connected ?? false;
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
</code_context>
<issue_to_address>
LocalEndPoint should likely be read-only.
Making LocalEndPoint settable after construction may cause inconsistencies, as TcpClient is initialized with this value. Recommend restricting modification to the constructor only.
Suggested implementation:
```csharp
public IPEndPoint LocalEndPoint { get; }
```
```csharp
public DefaultTcpSocketClient(string host, int port = 0)
{
LocalEndPoint = new IPEndPoint(GetIPAddress(host), port);
}
```
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:26` </location>
<code_context>
+
+ public IPEndPoint LocalEndPoint { get; set; }
+
+ [NotNull]
+ public ILogger<DefaultTcpSocketClient>? Logger { get; set; }
+
+ public int ReceiveBufferSize { get; set; } = 1024 * 10;
</code_context>
<issue_to_address>
Logger property is nullable but marked as [NotNull].
Please resolve the contradiction by either making Logger non-nullable and ensuring it is always set, or by removing the [NotNull] attribute if null is allowed.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:67` </location>
<code_context>
+ await _client.ConnectAsync(endPoint, token);
+
+ // 开始接收数据
+ _ = Task.Run(ReceiveAsync, token);
+
+ LocalEndPoint = (IPEndPoint)_client.Client.LocalEndPoint!;
</code_context>
<issue_to_address>
Fire-and-forget ReceiveAsync may hide exceptions.
Since exceptions from ReceiveAsync won't be observed, this could lead to crashes or lost errors. Capture the task and handle exceptions to ensure reliability.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:115` </location>
<code_context>
+
+ private async Task ReceiveAsync()
+ {
+ using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
+ var buffer = block.Memory;
+ _receiveCancellationTokenSource ??= new();
</code_context>
<issue_to_address>
Reusing the same buffer for multiple reads may cause issues.
Since 'buffer' is reassigned to a slice after each read, it may shrink with each iteration. Use a separate variable for the slice or reset 'buffer' to the full block each time to avoid this issue.
</issue_to_address>
### Comment 5
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketFactory.cs:18` </location>
<code_context>
+{
+ private readonly ConcurrentDictionary<string, ITcpSocketClient> _pool = new();
+
+ public ITcpSocketClient GetOrCreate(string host, int port = 0)
+ {
+ return _pool.GetOrAdd($"{host}:{port}", key =>
</code_context>
<issue_to_address>
Keying the pool by host:port string may cause collisions.
String keys like "host:port" can be ambiguous, particularly with IPv6 or hostnames containing colons. Use a tuple or struct as the key instead.
Suggested implementation:
```csharp
private readonly ConcurrentDictionary<(string host, int port), ITcpSocketClient> _pool = new();
public ITcpSocketClient GetOrCreate(string host, int port = 0)
{
return _pool.GetOrAdd((host, port), key =>
{
var client = new DefaultTcpSocketClient(key.host, key.port)
{
Logger = provider.GetService<ILogger<DefaultTcpSocketClient>>()
};
return client;
});
}
public ITcpSocketClient? Remove(string host, int port)
```
You should also update the Remove method to use the tuple key:
Replace any usage of $"{host}:{port}" with (host, port) in the Remove method implementation.
</issue_to_address>
### Comment 6
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:17` </location>
<code_context>
+/// <param name="length">The data package total data length.</param>
+public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
+{
+ private readonly Memory<byte> _data = new byte[length];
+
+ private int _receivedLength;
</code_context>
<issue_to_address>
Storing Memory<byte> for _data may lead to unexpected behavior.
Reusing the same Memory<byte> instance for multiple messages can cause data corruption if the callback retains a reference. Allocate a new buffer per message or ensure the callback does not keep the reference.
Suggested implementation:
```csharp
public class FixLengthDataPackageHandler(int length) : DataPackageHandlerBase
{
private int _receivedLength;
```
```csharp
public override async Task ReceiveAsync(Memory<byte> data)
{
// 处理上次粘包数据
data = ConcatBuffer(data);
// Allocate a new buffer for each message to avoid data corruption
while (data.Length > 0)
{
int bytesNeeded = length - _receivedLength;
int bytesToCopy = Math.Min(bytesNeeded, data.Length);
// Allocate a new buffer for each complete message
if (_receivedLength == 0 && data.Length >= length)
{
var messageBuffer = new byte[length];
data.Slice(0, length).CopyTo(messageBuffer);
await ProcessMessageAsync(messageBuffer);
data = data.Slice(length);
continue;
}
// Handle partial message
if (_receivedLength < length)
{
var tempBuffer = new byte[length];
// Copy previously received data if any
// (Assume you have a way to store partial data, e.g., a private field or method)
CopyPartialData(tempBuffer, _receivedLength);
data.Slice(0, bytesToCopy).CopyTo(tempBuffer.AsMemory(_receivedLength, bytesToCopy));
_receivedLength += bytesToCopy;
if (_receivedLength == length)
{
await ProcessMessageAsync(tempBuffer);
_receivedLength = 0;
ClearPartialData();
}
data = data.Slice(bytesToCopy);
}
}
```
- You will need to implement or adapt `CopyPartialData` and `ClearPartialData` to handle partial message storage, possibly using a private buffer for incomplete messages.
- Replace `ProcessMessageAsync` with your actual message processing logic.
- Remove any other code that references the old `_data` field.
- Adjust the logic if your actual message assembly is handled elsewhere.
</issue_to_address>
### Comment 7
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs:36` </location>
<code_context>
+ var segment = data.Length > len ? data[..len] : data;
+ segment.CopyTo(_data[_receivedLength..]);
+
+ if(data.Length > len)
+ {
+ HandlerStickyPackage(data, data.Length - len);
</code_context>
<issue_to_address>
HandlerStickyPackage may not handle multiple sticky packages.
Currently, only the first complete package is processed per call; any additional packages are deferred. Please update the logic to process all complete packages in the buffer within a single call.
</issue_to_address>
### Comment 8
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:77` </location>
<code_context>
+
+ // 测试未建立连接前调用 SendAsync 方法报异常逻辑
+ var data = new Memory<byte>([1, 2, 3, 4, 5]);
+ var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => client.SendAsync(data));
+ Assert.Equal("TCP Socket is not connected 127.0.0.1:0", ex.Message);
+ }
+
</code_context>
<issue_to_address>
Assertion on exception message is brittle.
Instead, assert on the exception type or check for key phrases in the message using `Assert.Contains` to make the test less fragile.
</issue_to_address>
### Comment 9
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:146` </location>
<code_context>
+ client.ReceiveBufferSize = 1024 * 20;
+ Assert.Equal(1024 * 20, client.ReceiveBufferSize);
+
+ client.SetDataHandler(new MockReceiveErrorHandler());
+ await client.ConnectAsync("localhost", port);
+
+ // 发送数据导致接收数据异常
+ var data = new Memory<byte>([1, 2, 3, 4, 5]);
+ await client.SendAsync(data);
+
+ // 关闭连接
</code_context>
<issue_to_address>
Covers error handling in data handler, but lacks assertion on error propagation.
Please add assertions to verify the expected behavior when an error occurs in the data handler, such as error logging, propagation, or client usability.
Suggested implementation:
```csharp
client.ReceiveBufferSize = 1024 * 20;
Assert.Equal(1024 * 20, client.ReceiveBufferSize);
// Set up a handler to capture/log errors
var errorHandler = new MockReceiveErrorHandler();
client.SetDataHandler(errorHandler);
await client.ConnectAsync("localhost", port);
// 发送数据导致接收数据异常
var data = new Memory<byte>([1, 2, 3, 4, 5]);
await client.SendAsync(data);
// 等待一段时间以确保错误被处理
await Task.Delay(100);
// 断言错误已被捕获/记录
Assert.True(errorHandler.ErrorHandled, "Error was not handled as expected in the data handler.");
// 断言客户端在错误后不可用（如已断开连接）
Assert.False(client.Connected, "Client should be disconnected after data handler error.");
StopTcpServer(server);
}
```
You will need to update `MockReceiveErrorHandler` to include an `ErrorHandled` property (e.g., a bool that is set to true when an error is handled in the data handler). If your client does not automatically disconnect on handler error, adjust the assertion accordingly or add logic to simulate/trigger disconnect.
</issue_to_address>
### Comment 10
<location> `test/UnitTest/Extensions/HostEnvironmentExtensionsTest.cs:14` </location>
<code_context>
+public class HostEnvironmentExtensionsTest
+{
+ [Fact]
+ public void IsWasm_Ok()
+ {
+ var hostEnvironment = new MockWasmHostEnvironment();
+ Assert.False(hostEnvironment.IsWasm());
+ }
+
</code_context>
<issue_to_address>
Test only covers the negative case for IsWasm.
Add a test for the positive case where IsWasm returns true, or document in the test if this isn't feasible due to environment limitations.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
public class HostEnvironmentExtensionsTest
{
[Fact]
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
=======
public class HostEnvironmentExtensionsTest
{
/// <summary>
/// Test only covers the negative case for IsWasm.
/// Positive case (IsWasm returns true) cannot be tested here due to environment limitations.
/// If a way to simulate a WASM environment is available, add a positive test here.
/// </summary>
[Fact]
public void IsWasm_Ok()
{
var hostEnvironment = new MockWasmHostEnvironment();
Assert.False(hostEnvironment.IsWasm());
}
// Example placeholder for a positive test, if feasible in the future:
// [Fact]
// public void IsWasm_ReturnsTrue_WhenInWasmEnvironment()
// {
// // Arrange: Setup a mock or environment that simulates WASM
// // var wasmHostEnvironment = new WasmHostEnvironmentStub(true);
// // Act & Assert
// // Assert.True(wasmHostEnvironment.IsWasm());
// }
>>>>>>> REPLACE
</suggested_fix>
### Comment 11
<location> `src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs:113` </location>
<code_context>
+ return ret;
+ }
+
+ private async Task ReceiveAsync()
+ {
+ using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
</code_context>
<issue_to_address>
Consider refactoring the receive and send methods by extracting smaller helper methods and a common error-handling wrapper to reduce nesting and improve code clarity.
```csharp
// You can reduce nesting and separate concerns by extracting small, focused methods.
// Example: split ReceiveAsync into a simple loop and a single-iteration handler:
private async Task ReceiveAsync()
{
using var block = MemoryPool<byte>.Shared.Rent(ReceiveBufferSize);
var buffer = block.Memory;
_receiveCancellationTokenSource ??= new();
while (!_receiveCancellationTokenSource.IsCancellationRequested)
{
await ReceiveIterationAsync(buffer);
}
}
private async Task ReceiveIterationAsync(Memory<byte> buffer)
{
EnsureConnected();
try
{
var stream = _client!.GetStream();
var length = await stream.ReadAsync(buffer, _receiveCancellationTokenSource!.Token);
if (length == 0)
{
Logger.LogInformation("Connection closed by {RemoteEndPoint}", _remoteEndPoint);
_receiveCancellationTokenSource.Cancel();
return;
}
var slice = buffer.Slice(0, length);
if (_dataPackageHandler != null)
{
await _dataPackageHandler.ReceiveAsync(slice);
}
}
catch (OperationCanceledException ex)
{
Logger.LogWarning(ex, "Receive canceled");
}
catch (Exception ex)
{
Logger.LogError(ex, "Receive failed");
}
}
private void EnsureConnected()
{
if (_client?.Connected != true)
throw new InvalidOperationException($"Not connected {LocalEndPoint}");
}
// Similarly, extract SendAsync logic:
public async Task<bool> SendAsync(Memory<byte> data, CancellationToken token = default)
=> await RunWithLoggingAsync(() => WriteDataAsync(data, token),
ex => Logger.LogError(ex, "Send failed"));
// and
private async Task<bool> WriteDataAsync(Memory<byte> data, CancellationToken token)
{
EnsureConnected();
if (_dataPackageHandler != null)
data = await _dataPackageHandler.SendAsync(data);
await _client!.GetStream().WriteAsync(data, token);
return true;
}
// Helper for consistent try/catch:
private async Task<bool> RunWithLoggingAsync(Func<Task<bool>> operation, Action<Exception> logError)
{
try
{
return await operation();
}
catch (OperationCanceledException ex)
{
Logger.LogWarning(ex, "Operation canceled");
return false;
}
catch (Exception ex)
{
logError(ex);
return false;
}
}
```
Steps:
1. Extract per-iteration receive logic into `ReceiveIterationAsync`.
2. Pull connection-check into `EnsureConnected()`.
3. Move send logic into `WriteDataAsync`.
4. Introduce a small `RunWithLoggingAsync` wrapper for try/catch.
5. Keep the public API unchanged; all behavior is preserved, nesting reduced.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DefaultTcpSocketClient.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataPackage/FixLengthDataPackageHandler.cs
Outdated
Show resolved
Hide resolved
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 #6253
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Implement a new TCP socket service with factory, client, and data package handling abstractions, along with DI extension and unit tests.
New Features:
Tests: