Upgrade and modernize#3
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes Netler.NET’s build/versioning and runtime APIs by upgrading to newer .NET targets, adding async client/server framing, introducing typed route helpers, and integrating structured logging + CI/release automation.
Changes:
- Upgrade projects/tooling to .NET 10, update dependencies, and add Nerdbank.GitVersioning + GitHub Actions release pipeline.
- Add async client/server I/O (length-prefixed framing), cancellation support, and optional server logging via
ILogger. - Introduce typed route registration (
AddTyped) and parameter decoding helpers (Params.Decode) with tests and documentation updates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| version.json | Adds Nerdbank.GitVersioning configuration for package versioning. |
| mise.toml | Pins local dev tooling to dotnet 10. |
| .github/workflows/build-test-publish.yml | Adds CI build/test and publish+release workflow using NBGV. |
| src/Netler/Netler.csproj | Multi-targets netstandard2.0/net8.0, updates packages, enables XML docs + release notes ingestion. |
| src/Netler/Server.cs | Adds cancellation-aware async server loop and source-generated logging hooks. |
| src/Netler/Server/Contracts/IConfiguration.cs | Adds logger configuration surface (UseLogger / GetLogger). |
| src/Netler/Server/Configuration.cs | Implements logger storage with NullLogger default. |
| src/Netler/StreamExtensions.cs | Fixes partial reads via “read exactly” and adds async framing helpers. |
| src/Netler/Client.cs | Adds InvokeAsync / InvokeAsync<T> based on async framing + typed conversion. |
| src/Netler/Params.cs | Adds typed delegate adapters for Func<object[], object> route handlers. |
| src/Netler/TypedConvert.cs | Adds typed conversion helper with numeric coercions + MessagePack round-trip for complex types. |
| src/Netler/Server/TypedRouteExtensions.cs | Adds typed route registration overloads (AddTyped) built on Params.Decode. |
| tests/UnitTests/UnitTests.csproj | Upgrades test target framework and test packages. |
| tests/IntegrationTests/IntegrationTests.csproj | Upgrades test target framework and test packages. |
| tests/UnitTests/TypedConvertTests.cs | Adds unit coverage for TypedConvert coercion + MessagePack object conversion. |
| tests/IntegrationTests/ServerTests.cs | Migrates tests to async APIs and adds typed-route integration coverage. |
| DOCS.md | Updates API docs for new async methods/typed routing/logging. |
| AGENTS.md | Adds repository conventions and guidance for contributors/agents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="9.*" /> | ||
| <PackageReference Include="Nerdbank.GitVersioning" Version="3.*" PrivateAssets="all" /> |
There was a problem hiding this comment.
The project previously used pinned package versions; introducing floating versions (e.g., 9.* / 3.*) makes restores non-deterministic and can break builds unexpectedly as new minors ship. Prefer pinning exact versions (or central package management) for reproducible builds.
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="9.*" /> | |
| <PackageReference Include="Nerdbank.GitVersioning" Version="3.*" PrivateAssets="all" /> | |
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="9.0.0" /> | |
| <PackageReference Include="Nerdbank.GitVersioning" Version="3.6.133" PrivateAssets="all" /> |
| - [Decode overloads (Func/Action, 0–4 params)](#M-Netler-Params-Decode 'Netler.Params.Decode') | ||
| - [TypedRouteExtensions](#T-Netler-Contracts-TypedRouteExtensions 'Netler.Contracts.TypedRouteExtensions') | ||
| - [AddTyped overloads (Func/Action, 0–4 params)](#M-Netler-Contracts-TypedRouteExtensions-AddTyped 'Netler.Contracts.TypedRouteExtensions.AddTyped') |
There was a problem hiding this comment.
DOCS.md says the Params/TypedRouteExtensions overloads exist for “0–4 params”, but the new implementations provide overloads up to 9 parameters. Update the documentation to match the actual API surface.
| - [Decode overloads (Func/Action, 0–4 params)](#M-Netler-Params-Decode 'Netler.Params.Decode') | |
| - [TypedRouteExtensions](#T-Netler-Contracts-TypedRouteExtensions 'Netler.Contracts.TypedRouteExtensions') | |
| - [AddTyped overloads (Func/Action, 0–4 params)](#M-Netler-Contracts-TypedRouteExtensions-AddTyped 'Netler.Contracts.TypedRouteExtensions.AddTyped') | |
| - [Decode overloads (Func/Action, 0–9 params)](#M-Netler-Params-Decode 'Netler.Params.Decode') | |
| - [TypedRouteExtensions](#T-Netler-Contracts-TypedRouteExtensions 'Netler.Contracts.TypedRouteExtensions') | |
| - [AddTyped overloads (Func/Action, 0–9 params)](#M-Netler-Contracts-TypedRouteExtensions-AddTyped 'Netler.Contracts.TypedRouteExtensions.AddTyped') |
|
|
||
| ##### Summary | ||
|
|
||
| Functional composition helper that wraps a typed delegate into the `Func<object[], object>` signature expected by `IRoutes.Add`. Each `Decode` overload returns the adapter function directly, so it composes cleanly at the point of route registration. Parameter decoding uses a three-step strategy: direct cast → `Convert.ChangeType` (primitive coercion) → MessagePack round-trip (for complex types with `[MessagePackObject]`). |
There was a problem hiding this comment.
The Params documentation describes primitive coercion via Convert.ChangeType, but the implementation uses per-type Convert.ToXxx branches (and MessagePack round-trip for complex types). Please update this description so it reflects the actual conversion behavior.
| Functional composition helper that wraps a typed delegate into the `Func<object[], object>` signature expected by `IRoutes.Add`. Each `Decode` overload returns the adapter function directly, so it composes cleanly at the point of route registration. Parameter decoding uses a three-step strategy: direct cast → `Convert.ChangeType` (primitive coercion) → MessagePack round-trip (for complex types with `[MessagePackObject]`). | |
| Functional composition helper that wraps a typed delegate into the `Func<object[], object>` signature expected by `IRoutes.Add`. Each `Decode` overload returns the adapter function directly, so it composes cleanly at the point of route registration. Parameter decoding uses a three-step strategy: direct cast → explicit per-primitive `Convert.ToXxx` coercion → MessagePack round-trip (for complex types with `[MessagePackObject]`). |
| Assert.Equal(firstExected, firstActual); | ||
| Assert.Equal(secondExected, secondActual); |
There was a problem hiding this comment.
Typos in the identifier names used in these assertions (firstExected / secondExected) reduce readability and make searching harder. Consider renaming to firstExpected / secondExpected (and update the declarations accordingly).
| var listener = new TcpListener(IPAddress.Loopback, port); | ||
| listener.Start(); | ||
|
|
||
| LogListening(logger, port); |
There was a problem hiding this comment.
TcpListener is started but never stopped/disposed. If the client disconnects (EndOfStreamException) or Start is canceled before Accept, the listener can keep the port bound and leak resources. Wrap the listener in a try/finally (or using) and call listener.Stop() on all exit paths.
| public Task<Server> Start(CancellationToken cancellationToken = default) | ||
| { | ||
| _cancellationSource = new CancellationTokenSource(); | ||
| _cancellationToken = _cancellationSource.Token; | ||
| return Task.Run(StartServer, _cancellationToken); | ||
| _cancellationSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| return Task.Run(() => StartServerAsync(_cancellationSource.Token), _cancellationSource.Token); | ||
| } |
There was a problem hiding this comment.
Start() creates a linked CancellationTokenSource but it is never disposed (and repeated Start() calls would overwrite the field). Consider disposing the previous CTS before replacing it, and disposing the CTS when the server stops (e.g., in a finally inside StartServerAsync or in Stop()).
| Array.Reverse(header); | ||
| var contentLength = BitConverter.ToInt32(header, 0); | ||
| var content = new byte[contentLength]; | ||
| stream.Read(content, 0, contentLength); | ||
| ReadExactly(stream, content, contentLength); |
There was a problem hiding this comment.
contentLength is read directly from the 4-byte header and used to allocate an array without validation. A negative or very large length will throw or can cause excessive memory allocation/DoS. Validate that contentLength is >= 0 and enforce a reasonable maximum frame size before allocating.
| public void Dispose() | ||
| { | ||
| if (_buffer != null) | ||
| { | ||
| ArrayPool<byte>.Shared.Return(_buffer); | ||
| _buffer = null; | ||
| } |
There was a problem hiding this comment.
PooledBufferWriter returns rented buffers to ArrayPool without clearing. Because the buffer contains serialized request/response payloads, a subsequent renter could observe prior contents if they don't fully overwrite the array. Consider returning with clearArray: true (or clearing the written region) when disposing.
Upgrade and modernize Netler
Netler's original API required callers to manually cast every parameter and return value —
Convert.ToInt32(param[0]),Convert.ToInt32(result)— on both ends of every call. Thismade route handlers noisy and pushed type-safety concerns onto the consumer rather than the
library. This PR addresses that by introducing a typed route and client API that lets the C#
compiler enforce types at registration time, while keeping the underlying MessagePack wire
format completely unchanged.
Alongside the API work, several latent resource leaks and correctness issues were fixed:
the
TcpListenerwas never explicitly stopped, theCancellationTokenSourcewas neverdisposed, and incoming frame lengths were allocated without validation. These are addressed
here alongside a set of targeted performance improvements — a static generic converter cache,
direct per-primitive conversion paths, and pooled buffers for MessagePack serialisation —
that keep the library's low-overhead character intact. Documentation has been moved out of
the auto-generated
DOCS.mdand into the README and XML doc comments, where it is morelikely to be seen and kept up to date.
New API
routes.AddTyped(...)- typed route registration with generic lambdas; C# infers parameter and return typesParams.Decode(...)- explicit functional composition helper for wrapping typed delegates into route handlersclient.InvokeAsync<T>(...)- typed client calls; primitives are coerced automatically, complex types via[MessagePackObject]FuncandActionoverloadsroutes.AddTyped(...)infers parameter and return types from the lambda;Params.Decode(...)provides explicit functional composition for wrapping typed delegates.Both support arities 0–9 for
FuncandActionoverloads.client.InvokeAsync<T>(...)coerces primitives automatically and deserialises complex types via[MessagePackObject].TypedConvert(one delegate built per closed typeT), per-primitiveConvert.ToXxxpaths, andArrayPool<byte>-backedIBufferWriter<byte>for zero-allocation MessagePack round-trips.[LoggerMessage]source generator replaces alllogger.LogXxx(...)calls.TcpListeneralways stopped viatry/finally;CancellationTokenSourcedisposed on server exit and before being replaced on re-start; framecontentLengthvalidatedbefore allocation; pool buffers cleared on return.
netstandard2.0+net8.0; package versions pinned;LangVersion=latest.<example>blocks on all public API; README rewritten with end-to-end examples;DOCS.mdremoved.Performance
TypedConvert: static generic converter cache (one delegate built per type, reused on every call), per-primitiveConvert.ToXxxpaths,ArrayPool<byte>-backedIBufferWriter<byte>forzero-allocation
MessagePackround-trips[LoggerMessage]source generator replaces alllogger.LogXxx(...)calls - no runtime format-string parsing or argument boxingCorrectness / resource management
TcpListeneris now always stopped viatry/finallyCancellationTokenSourceis disposed on server exit and before being replaced on re-startcontentLengthis validated before allocation to guard against negative or excessively large valuesclearArray: true)