Make the encoding providers' stream paths genuinely async - #22
Conversation
Addresses the non-breaking half of #8 for the encoding category: stream operations should do real asynchronous I/O rather than occupy a thread pool thread for the duration of a disk or network read. `Base64EncodingProvider` and `HexEncodingProvider` now declare the two `Try…Async(Stream, Stream, ...)` primitives themselves, which replaces `IEncodingProvider`'s `Task.Run` defaults. The interface's derived stream members — `Try…Async(ReadOnlyMemory, Stream)` and `…Async(Stream)` — are re-expressed over those primitives, the same way the compression interface already layers its own, so declaring two members converts six paths per provider. The transform itself is deliberately not offloaded. It is CPU work on a buffer already in memory, and whether that is worth a thread pool dispatch is the caller's decision — which is the other half of #8, still open and still a breaking choice. Each provider mirrors its own synchronous memory profile rather than adopting one shared shape: - Hex transforms a chunk at a time and keeps constant memory use. Hex maps one byte to two, so a chunk boundary never splits a pair when encoding. Decoding reads two characters per byte, so each read is topped up to an even length before transforming — a `ReadAsync` may legally return fewer bytes than asked for, and treating that as end-of-stream would silently truncate the output. - Base64 buffers the whole input, because it maps three bytes to four and a chunked transform would have to carry a partial group across every boundary. Its synchronous path already buffers, so this changes no memory characteristics. Tests cover a payload spanning multiple chunks, a stream that returns one byte per read (a `MemoryStream` never reads short, so it cannot catch that class of bug on its own), agreement between the derived overloads and the primitive, and cancellation. Reverting the short-read fill loop to a single read fails four of them. Obfuscation and serialization stream paths are unchanged. Serialization is blocked by #18; obfuscation is seven providers with differing stream shapes and is better done as its own change. README and CLAUDE.md updated to say which paths are now genuine. Full suite: 655 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
Coverage of the new code measured 64.9% the way Sonar computes it, which would have failed the quality gate. The gap was entirely the paths that report failure rather than succeed — exactly the ones a `Try` method exists for. Adds tests for null streams, input neither provider can decode, a stream that faults on read and on write, and a stream disposed before use. Also exercises `TryDecodeAsync(ReadOnlyMemory, Stream)`, which the derived-overload test previously covered only in the encode direction. Removes the `ArgumentException` catch from the two new Base64 async methods. Nothing in them can throw it: `CopyToAsync` validates its own buffer size, which is a constant here, and reports an unreadable stream as `NotSupportedException`, while `WriteAsync` over a valid range cannot throw it either. The synchronous path keeps its own catch, where the `Write(byte[], int, int)` overload can still raise one. New-code coverage 64.9% -> 92.3%. Full suite: 663 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
|
Pushed a second commit before this went to review, because I measured the coverage the quality gate uses and the first commit would not have passed it. New-code coverage was 64.9%, against a gate of 80%. The gap was entirely the paths that report failure rather than succeed — which is to say, the whole reason these are Added tests for null streams, input neither provider can decode, a stream that faults on read and separately on write (two different 64.9% → 92.3%. Full suite now 663 passed. One deletion worth flagging, since it is behaviour and not test scaffolding: the Also in the diff, and not mine: Generated by Claude Code |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
`Encoding_Async_ReportsADisposedStream` built a MemoryStream and disposed it by hand, so an assertion failure before that call would have leaked it. The stream is now held by an `await using` block with the explicit `DisposeAsync` inside it: the explicit call is the state under test, and the block guarantees disposal on every path out. MemoryStream.Dispose is idempotent, so disposing twice is a no-op. Written as a block with `ConfigureAwait(false)` on the resource rather than as an `await using` declaration, which CA2007 rejects in this project. Reported by github-code-quality on #22. Full suite: 663 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
The previous fix left the test holding a disposed local, which three analysers disagree about how to express. CodeQL wants an `await using` declaration; CA2007 rejects that declaration without ConfigureAwait, and wrapping the resource in ConfigureAwait is what hides the ownership from CodeQL in the first place; CA1849 rejects the synchronous Dispose that sidesteps both. The disagreement is a symptom rather than the problem. A test that needs an already-closed stream does not need to own a disposable at all, so building one moves into a helper that creates it, disposes it, and returns it. Disposal is then complete and local to the helper, and the test body holds an object whose lifetime is already over. Full suite: 663 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
|
2 participants
Addresses the non-breaking half of #8 for the encoding category.
#8 splits into two halves that pull in opposite directions. One is a decision only you can make — whether the in-memory operations keep an async surface at all, and at what cost in major versions. The other is not a decision at all:
That half was done for compression, AES and the two hash interfaces in #21. This does it for encoding. Obfuscation and serialization are still outstanding; see the end.
What changes
Base64EncodingProviderandHexEncodingProvidernow declare the twoTry…Async(Stream, Stream, ...)primitives themselves, which replacesIEncodingProvider'sTask.Rundefaults.IEncodingProvider's derived stream members —Try…Async(ReadOnlyMemory, Stream)and…Async(Stream)— are re-expressed over those primitives, exactly the wayICompressionProvideralready layers its own. So declaring two members converts six paths per provider, and any future encoding provider gets the same deal.The transform itself is deliberately not offloaded. It is CPU work on a buffer already in memory, and whether that is worth a thread pool dispatch is the caller's decision — which is the other half of #8, still open and still breaking.
Each provider mirrors its own memory profile
The two providers do not share one shape, because their synchronous paths don't either, and an async path that quietly changed a provider's memory characteristics would be a worse trade than the one it fixes.
Hex transforms a chunk at a time and keeps the constant memory use its synchronous path has. Hex maps one byte to two, so a chunk boundary never splits a pair when encoding.
Decoding is the part with a trap in it. It reads two characters per byte, so a chunk can end mid-pair — and separately,
ReadAsyncmay legally return fewer bytes than asked for at any point, which a network stream routinely does. A reader that treats a short read as end-of-stream silently truncates its output. So each read is topped up until the buffer holds an even number of characters or the stream has genuinely ended; a leftover character then means truncated input, and it reports failure exactly as the synchronous path does.Base64 buffers the whole input, because it maps three bytes to four and a chunked transform would have to carry a partial group across every boundary. Its synchronous path already buffers, so this changes nothing about its memory characteristics — only that the read and the write no longer hold a thread.
Testing
Four new tests per provider, covering what the existing short round-trip cannot:
MemoryStreamnever reads short, so no test built on one can catch the truncation bug above; this drip-feeds to force the case.OperationCanceledExceptionor itsTaskCanceledExceptionsubclass depending on the stream, and an exact-type assertion would be brittle.I checked these tests actually bite rather than just pass: reverting the short-read fill loop to a single
ReadAsyncfails four of them.Full suite: 655 passed, 0 warnings and 0 errors with the ktsu analyzers loaded.
What this does not do
#8 stays open, and deliberately:
ValueTask/ document as-is) are all breaking except the last, and the issue frames it as a decision rather than a task. I have not pre-empted it.ISerializationProvideris built onTextWriter/TextReader, andSystem.Text.Jsonhas no async overload for either — so its stream paths cannot become genuine without the signature redesign ISerializationProvider takes TextWriter/TextReader, which blocks genuine async serialization #18 describes.Xorstreams byte-wise,Reverseneeds the whole input,Base64/Hexdelegate to the encoders,Compositechains others. Converting them properly means a per-provider judgement like the one above, not one blanket edit, and folding that into this PR would bury it.README and
CLAUDE.mdare updated to say which paths are now genuine — the README claim was the one thing #8 says should be corrected regardless of which option is chosen.🤖 Generated with Claude Code
https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
Generated by Claude Code