Code-quality review: collapse PooledNntpClient, decompose NntpConnection, idiomatic cleanups (#154)#155
Merged
Merged
Conversation
Move the faulted-transport flag down into NntpConnection (set on read/write IO failures and lost/invalid responses), so the pool reads HasError straight from the connection. This removes the ~300 lines of hand-written per-command forwarders in PooledNntpClient: the pooled client collapses to a holder of the connection/auth state, and the lease exposes the underlying NntpClient directly as its IPooledNntpClient command surface. NntpConnection now tracks disposal and throws ObjectDisposedException once disposed, preserving the pool's post-error contract. LastActivity is stamped on return to the pool, which is the interval the idle monitor actually measures.
NntpConnection had crossed 1k lines by mixing transport/session, line framing and compression. Move the line-framing/data-block cluster (ReadLineAsync, TryReadLine, DecodeLine, ReadDataBlockToBufferAsync, AppendLine(s), EnsureCapacity, ProcessLine) into NntpLineFramer: pure byte->line logic over a swappable PipeReader, with its own read-byte counter and read-fault flag. The connection now holds a framer and swaps its Reader when rebuilding the transport for compression. HasError is OR'd from the connection's write/response faults and the framer's read faults. The framer is unit-tested over an in-memory pipe with no socket. NntpConnection drops from ~1015 to ~745 lines. Counter overflow now resets only the overflowing counter (read counting lives in the framer, writes in the connection), which also addresses the surprising both-counter reset noted in the review.
InstallDeflateLayer (session-wide DEFLATE, RFC 8054) and the per-command InstallDecompressionScopeAsync (ADR-0006) both wrapped any bytes the plaintext reader buffered past the status line in a PrefixStream over the live stream so a coalesced segment still decodes. Factor that subtle, drift-prone step into a shared ReplayInput helper; the codec choice (raw deflate vs the sniffed zlib/gzip/deflate switch) stays at each call site.
MultiValueDictionary inherited Dictionary<,> and shadowed Count with 'new', recomputing an O(n) flattened sum on every read and leaving a footgun for any code that touched it through the base type. It was only ever used transiently in NZB parsing/building to accumulate metadata before NzbDocument materialized an immutable copy, and its Equals/operators/JSON surface existed solely for its own tests. Drop the bespoke class (and its now-unused MultiSetComparer) in favour of a standard Dictionary<string, ICollection<string>> plus a small AddValue extension that creates the backing HashSet on first use. NzbDocument and its ToImmutableDictionaryWithHashSets conversion are unchanged.
- UsenetEncoding.Default now uses the static, allocation-free Encoding.Latin1
instead of the Encoding.GetEncoding("iso-8859-1") code-page lookup (byte-for-byte
identical on modern .NET).
- NzbParser.FileNameRegex and StringExtensions.WhitespaceRegex switch from
RegexOptions.Compiled to source-generated [GeneratedRegex] partial methods: no
runtime codegen, AOT-friendly and faster startup.
- ArticleResponseParser: split the assignment-in-expression (_successCode = (_requestType = requestType) switch ...) into two plain statements that are easier to scan. - NzbParser.ParseDocument: only re-query the nzb element when falling back to the no-namespace form, instead of always doing the lookup twice. (The duplicated dot-unstuffing rules now carry a cross-reference doc comment and the both-counter overflow reset was split per-counter as part of the framer extraction.)
The internal pool wrapper IInternalPooledNntpClient/PooledNntpClient was easily confused with the public command surface IPooledNntpClient (impl NntpClient), since the concrete PooledNntpClient implemented the *internal* interface, not IPooledNntpClient. Rename the pool-facing concept to read as a pool entry: - IInternalPooledNntpClient -> INntpPoolEntry - PooledNntpClient -> NntpPoolEntry - lease field _client -> _entry Public API (IPooledNntpClient, IPooledNntpClientLease, NntpClient) is unchanged.
Document the NntpPoolEntry concept (renamed from PooledNntpClient) in CONTEXT.md and docs/architecture.md so the ubiquitous-language glossary stays complete and the wrapper isn't reconfused with the public IPooledNntpClient command surface.
XZVER/XZHDR (compressed overview/header) were added to the composite INntpClient but never to IPooledNntpClient, so pooled-lease callers could not reach them without downcasting the leased client to NntpClient. These are plain command surfaces like the RFC interfaces the lease already exposes (the pool only deliberately withholds auth and connection control), and compressed overview is exactly what pooled consumers fetch. Add INntpClientXz to IPooledNntpClient. NntpClient already implements it, so no other changes are needed; the public API tracking files are unaffected.
The namespace cleanup moved several public types (e.g. NntpHeaders -> Usenet.Nntp.Models) and recorded the new-namespace entries in PublicAPI.Unshipped.txt, but the corresponding old-namespace entries were never removed from PublicAPI.Shipped.txt. On a clean build this left 154 stale RS0017 entries (symbol declared in the API file but no longer found). Promote all unshipped entries into Shipped, apply the *REMOVED* markers, and drop the 154 stale old-namespace lines. Verified against the analyzer as the source of truth: clean build now reports RS0016=0 and RS0017=0 for both net8.0 and net10.0. Unshipped is reset to the bare header.
The namespace-cleanup commit (1ffef2c) garbled three predicates in the pool, apparently from a bad find/replace: - ReturnClient dropped errored connections via HasError, but the check had become 'HasPendingStream || HasPendingStream', so a connection that errored (e.g. server reset mid-command) was handed back to the pool instead of being disposed. This regressed DisposeClientAfterError. - BorrowClient connected only when '!Authenticated' instead of '!Connected', and the post-conditions/log read 'Authenticated' where 'Connected' was meant. Restore the intended Connected/HasError logic. Full test suite green (308).
SummarySummary
CoverageUsenet - 75.8%
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #154.
Works through the code-quality review in order of structural impact, one concern per commit.
1. Collapse
PooledNntpClientdelegation, moveHasErrorintoNntpConnectionThe pooled client was ~300 lines of hand-written per-command forwarders kept in lockstep with
INntpClient, existing only to flipHasErroron a throw and gate access. The faulted-transport flag now lives inNntpConnection(set on read/write IO failures and lost/invalid responses), so:HasErrorcomes from the connection for free.PooledNntpClientcollapses to a holder of connection/auth state.NntpClientdirectly as itsIPooledNntpClientsurface.NntpConnectionnow tracks disposal and throwsObjectDisposedExceptiononce disposed (preserving the pool's post-error contract), andLastActivityis stamped on return to the pool — the interval the idle monitor actually measures.2. Extract line framing into a socket-free
NntpLineFramerNntpConnectionhad crossed 1k lines mixing transport/session, line framing and compression. The line-framing/data-block cluster moves intoNntpLineFramer: pure byte→line logic over a swappablePipeReader, with its own read-byte counter and read-fault flag, unit-tested over an in-memory pipe with no socket.NntpConnectiondrops from ~1015 to ~745 lines. Counter overflow now resets only the overflowing counter.3. De-duplicate the over-read replay in the two compression-install paths
The session-wide DEFLATE layer (RFC 8054) and the per-command decompression scope (ADR-0006) both replayed bytes over-read past the status line through a
PrefixStream. That drift-prone step is now a sharedReplayInputhelper; the codec choice stays at each call site.4. Replace
MultiValueDictionarywith a plain set-valued dictionaryThe bespoke type inherited
Dictionary<,>, shadowedCountwithnew(an O(n) recompute and a base-type footgun), and carriedEquals/operators/JSON only for its own tests. It was only used transiently to accumulate NZB metadata beforeNzbDocumentmade an immutable copy. Replaced with a standardDictionary<string, ICollection<string>>plus a smallAddValueextension; the now-unusedMultiSetCompareris removed too.5. Idiomatic modernizations
UsenetEncoding.Default→Encoding.Latin1(no code-page lookup).NzbParser/StringExtensionsregexes → source-generated[GeneratedRegex].IAsyncDisposablesuggestion for the gracefulQUITis deliberately deferred: making the pooled-client dispose async ripples intoIPooledNntpClientLease/the pool's documented dispose-outside-lock threading and touches public API, which is out of scope for a local cleanup.6. Smaller readability cleanups
ArticleResponseParserctor: split the assignment-in-expression into two statements.NzbParser.ParseDocument: only re-query the nzb element on the no-namespace fallback.Verification
dotnet csharpier check .clean.dotnet build -c Releaseanddotnet test -c Release: 308 tests pass (incl. newNntpLineFramerTests).