Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request aims to improve the performance of TCP socket send/receive operations by refactoring the internal implementation. The key changes focus on simplifying the API and reducing memory allocations.
- Removed logging infrastructure and dependency injection complexity
- Simplified
ReceiveAsyncAPI to use buffer-based approach instead of returningMemory<byte> - Streamlined internal
SenderandReceiverclasses by removing memory pooling and pinning overhead
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| UnitTestTcpSocket.csproj | Added Microsoft.Extensions.DependencyInjection package dependency |
| TcpSocketFactoryTest.cs | Updated test API calls to match new buffer-based ReceiveAsync, commented out many tests |
| SocketClientProviderTest.cs | Updated test to use new buffer-based ReceiveAsync API |
| TcpSocketClientOptions.cs | Removed logging options and changed NoDelay default |
| Longbow.TcpSocket.csproj | Updated version, removed logging dependencies, pinned Longbow.Sockets version |
| Sender.cs | Simplified by removing memory pinning and state management |
| Receiver.cs | Removed array pooling and simplified buffer handling |
| ITcpSocketClient.cs | Changed ReceiveAsync signature to buffer-based approach |
| DefaultTcpSocketClient.cs | Major refactor removing DI, logging, and reconnection logic |
| Service extensions | Updated to use Options pattern and simplified registration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Longbow.Sockets" Version="9.*" /> | ||
| <PackageReference Include="Longbow.Sockets" Version="9.0.2" /> |
There was a problem hiding this comment.
The version is now pinned to a specific version (9.0.2) instead of the previous wildcard pattern (9.*). This removes flexibility for patch updates and may require manual updates for bug fixes in the dependency.
| <PackageReference Include="Longbow.Sockets" Version="9.0.2" /> | |
| <PackageReference Include="Longbow.Sockets" Version="9.0.*" /> |
| if (e.SocketError == SocketError.Success) | ||
| { | ||
| tcs.TrySetResult(); | ||
| tcs.TrySetResult(true); |
There was a problem hiding this comment.
The method now returns true for successful sends, but the return type changed from ValueTask to ValueTask<bool>. However, there's no handling for partial sends - if SendAsync only sends part of the data, this will still return true even though not all data was sent.
| tcs.TrySetResult(true); | |
| // Check for partial send | |
| if (e.BytesTransferred == e.Count) | |
| { | |
| tcs.TrySetResult(true); | |
| } | |
| else | |
| { | |
| // Partial send: not all data was sent | |
| tcs.TrySetResult(false); | |
| } |
| } | ||
|
|
||
| _args.UserToken = (state, registration); | ||
| _args.SetBuffer(buffer); |
There was a problem hiding this comment.
The receiver now directly uses the provided buffer instead of using array pooling. This could lead to performance issues if the caller provides a very large buffer for small receives, as the entire buffer is set on the SocketAsyncEventArgs instead of using an appropriately-sized pooled buffer.
| await _socketProvider.CloseAsync(); | ||
| } | ||
| } | ||
| public ValueTask<int> ReceiveAsync(Memory<byte> buffer, CancellationToken token = default) => ReceiveCoreAsync(buffer, token); |
There was a problem hiding this comment.
The public ReceiveAsync method doesn't validate if auto-receive is enabled. The removed validation that threw InvalidOperationException when IsAutoReceive is true means users could now incorrectly call both manual and automatic receive simultaneously, leading to undefined behavior.
Link issues
fixes #25
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge