From 1ba8f99f2e7a965341a2189031e34050423d8014 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 12:05:46 +0000 Subject: [PATCH 1/4] feat: make the encoding providers' stream paths genuinely async [minor] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg --- CLAUDE.md | 4 +- .../Base64EncodingProvider.cs | 105 +++++++++++++ .../HexEncodingProvider.cs | 147 ++++++++++++++++++ Essentials.Tests/EncodingProviderTests.cs | 144 +++++++++++++++++ Essentials/IEncodingProvider.cs | 36 ++++- README.md | 2 +- 6 files changed, 428 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f72862f..21a73c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,7 +88,9 @@ All provider interfaces follow a consistent three-tier pattern: 1. **Core Try\* methods**: Buffer-based methods over `Span` or `Stream`. Span overloads are `bool TryX(source, destination, out int bytesWritten)`, paired with a `GetMax…Length` bound per category so callers can size buffers. These are the only methods implementers must provide. 2. **Convenience methods**: Self-allocating methods that call Try\* methods and manage buffers automatically. Provided via default interface implementations. -3. **Async variants**: Task-based async versions with `CancellationToken` support. The stream paths of the compression providers and of `AesEncryptionProvider`, along with `IHashProvider.TryHashAsync(Stream, ...)` and `IKeyedHashProvider.TryHashAsync(ReadOnlyMemory, Stream, ...)`, are genuinely asynchronous — real `ReadAsync`/`WriteAsync`, no thread held. The rest are still `Task.Run` wrappers over synchronous work via `ProviderHelpers.RunAsync()`; see issue #8. A provider makes its stream paths genuine by declaring the two `Try…Async(Stream, Stream, ...)` primitives itself, which replaces the default implementation; the four derived stream defaults compose over those primitives, so overriding two members converts all six. Span-destination async overloads do not exist — an `out` parameter cannot cross an async boundary. +3. **Async variants**: Task-based async versions with `CancellationToken` support. The stream paths of the compression providers, of `AesEncryptionProvider` and of both encoding providers, along with `IHashProvider.TryHashAsync(Stream, ...)` and `IKeyedHashProvider.TryHashAsync(ReadOnlyMemory, Stream, ...)`, are genuinely asynchronous — real `ReadAsync`/`WriteAsync`, no thread held. The rest — obfuscation, serialization, and everything operating on buffers already in memory — are still `Task.Run` wrappers over synchronous work via `ProviderHelpers.RunAsync()`; see issue #8. + + The two encoding providers each mirror their own synchronous memory profile rather than sharing one shape. `HexEncodingProvider` transforms a chunk at a time, because hex maps one byte to two and a chunk boundary never splits a pair; its decoder tops each read up to an even length first, since a `ReadAsync` may legally return fewer bytes than asked for and treating that as end-of-stream would truncate. `Base64EncodingProvider` buffers the whole input, because Base64 maps three bytes to four and a chunked transform would have to carry a partial group across every boundary — which is what its synchronous path does too. A provider makes its stream paths genuine by declaring the two `Try…Async(Stream, Stream, ...)` primitives itself, which replaces the default implementation; the four derived stream defaults compose over those primitives, so overriding two members converts all six. Span-destination async overloads do not exist — an `out` parameter cannot cross an async boundary. `ICommandExecutor` is the one interface with the mirror-image concern: synchronous methods layered over an asynchronous one. It declares a synchronous primitive, `Execute(string, IReadOnlyDictionary?, diff --git a/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs b/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs index 2d22b73..540a22a 100644 --- a/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs +++ b/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs @@ -6,6 +6,8 @@ namespace ktsu.Essentials.EncodingProviders.Base64; using System; using System.Buffers; using System.IO; +using System.Threading; +using System.Threading.Tasks; using SysBase64 = System.Buffers.Text.Base64; /// @@ -126,4 +128,107 @@ public bool TryDecode(Stream encodedData, Stream destination) return false; } } + + /// + /// + /// Genuinely asynchronous: the source is read and the result written with ReadAsync and + /// WriteAsync, so no thread is held for the duration of the I/O. Declaring this member and + /// its decoding counterpart replaces the interface's Task.Run defaults and converts every + /// stream path derived from them. + /// + /// The whole input is buffered before the transform, which is what the synchronous path does too: + /// Base64 encodes three bytes to four, so a chunked transform would have to carry a partial group + /// across every boundary. The transform itself is CPU work on a buffer already in memory and is + /// deliberately not offloaded — that is the caller's decision to make, not this provider's. + /// + /// + public async Task TryEncodeAsync(Stream data, Stream destination, CancellationToken cancellationToken = default) + { + if (data is null || destination is null) + { + return false; + } + + try + { + byte[] source = await ReadAllAsync(data, cancellationToken).ConfigureAwait(false); + byte[] encoded = new byte[GetMaxEncodedLength(source.Length)]; + + if (!TryEncode(source, encoded, out int bytesWritten)) + { + return false; + } + + await destination.WriteAsync(encoded.AsMemory(0, bytesWritten), cancellationToken).ConfigureAwait(false); + return true; + } + catch (ArgumentException) + { + return false; + } + catch (IOException) + { + return false; + } + catch (ObjectDisposedException) + { + return false; + } + } + + /// + /// + /// Genuinely asynchronous, on the same terms as . + /// + public async Task TryDecodeAsync(Stream encodedData, Stream destination, CancellationToken cancellationToken = default) + { + if (encodedData is null || destination is null) + { + return false; + } + + try + { + byte[] source = await ReadAllAsync(encodedData, cancellationToken).ConfigureAwait(false); + byte[] decoded = new byte[GetMaxDecodedLength(source.Length)]; + + if (!TryDecode(source, decoded, out int bytesWritten)) + { + return false; + } + + await destination.WriteAsync(decoded.AsMemory(0, bytesWritten), cancellationToken).ConfigureAwait(false); + return true; + } + catch (ArgumentException) + { + return false; + } + catch (IOException) + { + return false; + } + catch (ObjectDisposedException) + { + return false; + } + } + + /// + /// Reads a stream to its end without holding a thread. + /// + /// The stream to read. + /// The cancellation token. + /// Everything the stream had left. + private static async Task ReadAllAsync(Stream source, CancellationToken cancellationToken) + { + using MemoryStream buffer = new(); + await source.CopyToAsync(buffer, CopyBufferSize, cancellationToken).ConfigureAwait(false); + return buffer.ToArray(); + } + + /// + /// The chunk size used when reading a source stream, matching the framework's own default. + /// + private const int CopyBufferSize = 81920; } diff --git a/Essentials.EncodingProviders.Hex/HexEncodingProvider.cs b/Essentials.EncodingProviders.Hex/HexEncodingProvider.cs index a4169c3..686c2f6 100644 --- a/Essentials.EncodingProviders.Hex/HexEncodingProvider.cs +++ b/Essentials.EncodingProviders.Hex/HexEncodingProvider.cs @@ -5,6 +5,8 @@ namespace ktsu.Essentials.EncodingProviders.Hex; using ktsu.Essentials; using System; using System.IO; +using System.Threading; +using System.Threading.Tasks; /// /// An encoding provider that uses hexadecimal encoding for data encoding and decoding. @@ -148,4 +150,149 @@ private static bool TryParseNibble(byte character, out int value) return value >= 0; } + + /// + /// + /// Genuinely asynchronous: the source is read and the result written with ReadAsync and + /// WriteAsync, so no thread is held for the duration of the I/O. Declaring this member and + /// its decoding counterpart replaces the interface's Task.Run defaults and converts every + /// stream path derived from them. + /// + /// It transforms a chunk at a time rather than buffering the whole stream, which keeps the + /// synchronous path's constant memory use. Hex encodes one byte to two, so a chunk boundary never + /// splits a group and no state has to be carried across one. + /// + /// + public async Task TryEncodeAsync(Stream data, Stream destination, CancellationToken cancellationToken = default) + { + if (data is null || destination is null) + { + return false; + } + + byte[] source = new byte[ChunkSize]; + byte[] encoded = new byte[ChunkSize * 2]; + + try + { + int read; + while ((read = await data.ReadAsync(source.AsMemory(0, ChunkSize), cancellationToken).ConfigureAwait(false)) > 0) + { + for (int i = 0; i < read; i++) + { + encoded[i * 2] = (byte)HexDigits[source[i] >> 4]; + encoded[(i * 2) + 1] = (byte)HexDigits[source[i] & 0x0F]; + } + + await destination.WriteAsync(encoded.AsMemory(0, read * 2), cancellationToken).ConfigureAwait(false); + } + + return true; + } + catch (IOException) + { + return false; + } + catch (ObjectDisposedException) + { + return false; + } + } + + /// + /// + /// Genuinely asynchronous, and chunked, on the same terms as + /// . + /// + /// Decoding reads two characters per byte, so a chunk can end mid-pair. A read is therefore + /// topped up until it holds an even number of characters, or the stream ends — in which case a + /// leftover character means the input was truncated, and this reports failure exactly as the + /// synchronous path does. + /// + /// + public async Task TryDecodeAsync(Stream encodedData, Stream destination, CancellationToken cancellationToken = default) + { + if (encodedData is null || destination is null) + { + return false; + } + + byte[] source = new byte[ChunkSize]; + byte[] decoded = new byte[ChunkSize / 2]; + + try + { + while (true) + { + int filled = await ReadPairsAsync(encodedData, source, cancellationToken).ConfigureAwait(false); + if (filled == 0) + { + return true; + } + + if (filled < 0) + { + // An odd number of characters: the last byte has no partner, so the input is truncated. + return false; + } + + for (int i = 0; i < filled; i += 2) + { + if (!TryParseNibble(source[i], out int high) || !TryParseNibble(source[i + 1], out int low)) + { + return false; + } + + decoded[i / 2] = (byte)((high << 4) | low); + } + + await destination.WriteAsync(decoded.AsMemory(0, filled / 2), cancellationToken).ConfigureAwait(false); + } + } + catch (IOException) + { + return false; + } + catch (ObjectDisposedException) + { + return false; + } + } + + /// + /// Fills a buffer with an even number of characters, so no character pair straddles a chunk. + /// + /// The stream to read. + /// The buffer to fill. + /// The cancellation token. + /// + /// How many characters were read, zero at a clean end of stream, or -1 if the stream ended on an + /// unpaired character. + /// + /// + /// A single ReadAsync may return fewer bytes than asked for at any time — that is the + /// stream contract, not an end-of-stream signal — so this keeps reading until the buffer is full + /// or the stream really has ended. + /// + private static async Task ReadPairsAsync(Stream source, byte[] buffer, CancellationToken cancellationToken) + { + int filled = 0; + while (filled < buffer.Length) + { + int read = await source.ReadAsync(buffer.AsMemory(filled, buffer.Length - filled), cancellationToken).ConfigureAwait(false); + if (read == 0) + { + break; + } + + filled += read; + } + + return filled % 2 == 0 ? filled : -1; + } + + /// + /// The number of characters transformed per chunk. Even, so a pair is never split. + /// + private const int ChunkSize = 8192; } diff --git a/Essentials.Tests/EncodingProviderTests.cs b/Essentials.Tests/EncodingProviderTests.cs index bc3d214..8d81ad4 100644 --- a/Essentials.Tests/EncodingProviderTests.cs +++ b/Essentials.Tests/EncodingProviderTests.cs @@ -2,8 +2,12 @@ namespace ktsu.Essentials.Tests; +using System; using System.Collections.Generic; +using System.IO; using System.Text; +using System.Threading; +using System.Threading.Tasks; using ktsu.Essentials; using Microsoft.Extensions.DependencyInjection; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -100,4 +104,144 @@ public void Encoding_Async_Roundtrip(IEncodingProvider encoder, string providerN CollectionAssert.AreEqual(original, decodedStream.ToArray(), $"{providerName} async should produce original data"); } + + /// + /// Tests the async stream paths over an input large enough to cross a provider's internal chunk + /// boundary, which the short round-trip above never reaches. + /// + /// + /// The chunked provider is the one this matters for: a boundary that split a character pair, or a + /// short read treated as end-of-stream, would corrupt or truncate the output, and neither shows up + /// on a payload that fits in one chunk. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_RoundtripsAcrossChunkBoundaries(IEncodingProvider encoder, string providerName) + { + byte[] original = new byte[40_000]; + for (int i = 0; i < original.Length; i++) + { + original[i] = (byte)(i % 251); + } + + using MemoryStream inputStream = new(original); + using MemoryStream encodedStream = new(); + Assert.IsTrue( + await encoder.TryEncodeAsync(inputStream, encodedStream, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} async should encode a multi-chunk payload"); + + encodedStream.Position = 0; + using MemoryStream decodedStream = new(); + Assert.IsTrue( + await encoder.TryDecodeAsync(encodedStream, decodedStream, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} async should decode a multi-chunk payload"); + + CollectionAssert.AreEqual(original, decodedStream.ToArray(), $"{providerName} async should survive a multi-chunk round trip"); + } + + /// + /// Tests that a short read part-way through a stream is treated as "more to come" rather than as + /// the end of it. + /// + /// + /// A stream may legally return fewer bytes than asked for at any point — a network stream routinely + /// does — and a chunked reader that mistakes that for end-of-stream silently truncates. A + /// never does it, so it cannot catch this; this drip-feeds a byte at a + /// time to force the case. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_SurvivesShortReads(IEncodingProvider encoder, string providerName) + { + byte[] original = Encoding.UTF8.GetBytes("a payload delivered one byte at a time, via " + providerName); + + using MemoryStream encodedStream = new(); + using (DripStream input = new(original)) + { + Assert.IsTrue( + await encoder.TryEncodeAsync(input, encodedStream, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} async should encode from a stream that reads short"); + } + + byte[] encoded = encodedStream.ToArray(); + using MemoryStream decodedStream = new(); + using (DripStream input = new(encoded)) + { + Assert.IsTrue( + await encoder.TryDecodeAsync(input, decodedStream, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} async should decode from a stream that reads short"); + } + + CollectionAssert.AreEqual(original, decodedStream.ToArray(), $"{providerName} async should not truncate on short reads"); + } + + /// + /// Tests that the memory-source and self-allocating stream overloads reach the same result as the + /// stream-to-stream primitive, since they are now layered over it rather than over the synchronous + /// path. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_DerivedOverloadsAgreeWithThePrimitive(IEncodingProvider encoder, string providerName) + { + byte[] original = Encoding.UTF8.GetBytes("derived overloads for " + providerName); + + using MemoryStream fromMemory = new(); + Assert.IsTrue( + await encoder.TryEncodeAsync(original.AsMemory(), fromMemory, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} async should encode from memory to a stream"); + + using MemoryStream source = new(original); + byte[] selfAllocated = await encoder.EncodeAsync(source, TestContext.CancellationToken).ConfigureAwait(false); + + CollectionAssert.AreEqual(fromMemory.ToArray(), selfAllocated, $"{providerName} overloads should agree"); + + using MemoryStream encoded = new(selfAllocated); + byte[] decoded = await encoder.DecodeAsync(encoded, TestContext.CancellationToken).ConfigureAwait(false); + CollectionAssert.AreEqual(original, decoded, $"{providerName} should decode what it encoded"); + } + + /// + /// Tests that a cancelled token stops the async stream paths rather than being ignored. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_HonoursCancellation(IEncodingProvider encoder, string providerName) + { + byte[] original = new byte[40_000]; + using CancellationTokenSource cancelled = new(); + await cancelled.CancelAsync().ConfigureAwait(false); + + using MemoryStream input = new(original); + using MemoryStream output = new(); + + // Caught by base type on purpose: the framework may surface either OperationCanceledException + // or its TaskCanceledException subclass, and which one depends on the stream implementation. + // An exact-type assertion would be brittle here. + bool observed = false; + try + { + await encoder.TryEncodeAsync(input, output, cancelled.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + observed = true; + } + + Assert.IsTrue(observed, $"{providerName} should observe a cancelled token"); + } + + /// + /// A stream that returns one byte per read, however much is asked for. + /// + /// What the stream holds. + private sealed class DripStream(byte[] content) : MemoryStream(content) + { + public override int Read(byte[] buffer, int offset, int count) => base.Read(buffer, offset, Math.Min(count, 1)); + + public override int Read(Span buffer) => base.Read(buffer[..Math.Min(buffer.Length, 1)]); + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + => base.ReadAsync(buffer[..Math.Min(buffer.Length, 1)], cancellationToken); + } } diff --git a/Essentials/IEncodingProvider.cs b/Essentials/IEncodingProvider.cs index 49321fd..960800a 100644 --- a/Essentials/IEncodingProvider.cs +++ b/Essentials/IEncodingProvider.cs @@ -92,8 +92,13 @@ public string Encode(string data) /// The destination stream to write the encoded data to. /// The cancellation token. /// True if the encoding was successful, false otherwise. - public Task TryEncodeAsync(ReadOnlyMemory data, Stream destination, CancellationToken cancellationToken = default) - => ProviderHelpers.RunAsync(() => TryEncode(data.Span, destination), cancellationToken); + public async Task TryEncodeAsync(ReadOnlyMemory data, Stream destination, CancellationToken cancellationToken = default) + { + using MemoryStream source = new(); + await source.WriteAsync(data, cancellationToken).ConfigureAwait(false); + source.Position = 0; + return await TryEncodeAsync(source, destination, cancellationToken).ConfigureAwait(false); + } /// /// Tries to encode the data from the stream and write the result to the destination stream asynchronously. @@ -120,8 +125,13 @@ public Task EncodeAsync(ReadOnlyMemory data, CancellationToken can /// The data to encode. /// The cancellation token. /// The encoded data. - public Task EncodeAsync(Stream data, CancellationToken cancellationToken = default) - => ProviderHelpers.RunAsync(() => Encode(data), cancellationToken); + public async Task EncodeAsync(Stream data, CancellationToken cancellationToken = default) + { + using MemoryStream destination = new(); + return !await TryEncodeAsync(data, destination, cancellationToken).ConfigureAwait(false) + ? throw new InvalidOperationException("Encoding failed to produce output with the allocated buffer.") + : destination.ToArray(); + } /// /// Encodes the data from the string and returns the result asynchronously. @@ -195,8 +205,13 @@ public string Decode(string encodedData) /// The destination stream to write the decoded data to. /// The cancellation token. /// True if the decoding was successful, false otherwise. - public Task TryDecodeAsync(ReadOnlyMemory encodedData, Stream destination, CancellationToken cancellationToken = default) - => ProviderHelpers.RunAsync(() => TryDecode(encodedData.Span, destination), cancellationToken); + public async Task TryDecodeAsync(ReadOnlyMemory encodedData, Stream destination, CancellationToken cancellationToken = default) + { + using MemoryStream source = new(); + await source.WriteAsync(encodedData, cancellationToken).ConfigureAwait(false); + source.Position = 0; + return await TryDecodeAsync(source, destination, cancellationToken).ConfigureAwait(false); + } /// /// Tries to decode the data from the stream and write the result to the destination stream asynchronously. @@ -223,8 +238,13 @@ public Task DecodeAsync(ReadOnlyMemory encodedData, CancellationTo /// The encoded data to decode. /// The cancellation token. /// The decoded data. - public Task DecodeAsync(Stream encodedData, CancellationToken cancellationToken = default) - => ProviderHelpers.RunAsync(() => Decode(encodedData), cancellationToken); + public async Task DecodeAsync(Stream encodedData, CancellationToken cancellationToken = default) + { + using MemoryStream destination = new(); + return !await TryDecodeAsync(encodedData, destination, cancellationToken).ConfigureAwait(false) + ? throw new InvalidOperationException("Decoding failed to produce output with the allocated buffer.") + : destination.ToArray(); + } /// /// Decodes text produced by asynchronously. diff --git a/README.md b/README.md index b84ec15..a337491 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ - **Filesystem**: `IFileSystemProvider` extending Testably.Abstractions for testable filesystem access - **Explicit Buffer Contract**: every span operation is `bool TryX(source, destination, out int bytesWritten)` and each category exposes a `GetMax…Length` bound, so callers can size a buffer up front and know exactly how much was written. Encoding, hashing and obfuscation run allocation-free on the span path; compression and encryption still buffer internally, because the underlying BCL APIs for those are stream-only - **Minimal Implementation Burden**: Default interface implementations reduce boilerplate — implement only the core `Try*` methods -- **Async Support**: Operations expose async variants with `CancellationToken` support. Stream hashing, keyed hash stream hashing, and the stream paths of the compression and AES encryption providers, are genuinely asynchronous — they read and write with `ReadAsync`/`WriteAsync` and hold no thread. The encoding, obfuscation, serialization and in-memory variants are convenience wrappers that run synchronous work on the thread pool; span-destination operations have no async form, because an `out` parameter cannot cross an await boundary +- **Async Support**: Operations expose async variants with `CancellationToken` support. Stream hashing, keyed hash stream hashing, and the stream paths of the compression, AES encryption and encoding providers, are genuinely asynchronous — they read and write with `ReadAsync`/`WriteAsync` and hold no thread. The obfuscation, serialization and in-memory variants are convenience wrappers that run synchronous work on the thread pool; span-destination operations have no async form, because an `out` parameter cannot cross an await boundary - **Batteries-Included or Cherry-Pick**: Each provider ships as its own `ktsu.Essentials..` package; install the `ktsu.Essentials.All` meta-package to get every provider at once, or reference only the ones you need ## Installation From 9b3284c156eee9dd1a089885099e7479540346a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 12:11:17 +0000 Subject: [PATCH 2/4] test: cover the encoding providers' async failure paths [patch] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg --- .editorconfig | 2 +- .../Base64EncodingProvider.cs | 8 - Essentials.Tests/EncodingProviderTests.cs | 137 ++++++++++++++++++ 3 files changed, 138 insertions(+), 9 deletions(-) diff --git a/.editorconfig b/.editorconfig index 2cd5f5b..fb1feb2 100644 --- a/.editorconfig +++ b/.editorconfig @@ -382,7 +382,7 @@ dotnet_naming_style.pascal_case.word_separator = dotnet_naming_style.pascal_case.capitalization = pascal_case # C++ files -[*.cpp,*.h,*.hpp,*.cc,*.hh,*.cxx,*.hxx] +[*.{cpp,h,hpp,cc,hh,cxx,hxx}] indent_style = tab # Naming convention rules (note: currently need to be ordered from more to less specific) diff --git a/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs b/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs index 540a22a..f9ed1fd 100644 --- a/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs +++ b/Essentials.EncodingProviders.Base64/Base64EncodingProvider.cs @@ -162,10 +162,6 @@ public async Task TryEncodeAsync(Stream data, Stream destination, Cancella await destination.WriteAsync(encoded.AsMemory(0, bytesWritten), cancellationToken).ConfigureAwait(false); return true; } - catch (ArgumentException) - { - return false; - } catch (IOException) { return false; @@ -200,10 +196,6 @@ public async Task TryDecodeAsync(Stream encodedData, Stream destination, C await destination.WriteAsync(decoded.AsMemory(0, bytesWritten), cancellationToken).ConfigureAwait(false); return true; } - catch (ArgumentException) - { - return false; - } catch (IOException) { return false; diff --git a/Essentials.Tests/EncodingProviderTests.cs b/Essentials.Tests/EncodingProviderTests.cs index 8d81ad4..cdefc36 100644 --- a/Essentials.Tests/EncodingProviderTests.cs +++ b/Essentials.Tests/EncodingProviderTests.cs @@ -199,6 +199,12 @@ await encoder.TryEncodeAsync(original.AsMemory(), fromMemory, TestContext.Cancel using MemoryStream encoded = new(selfAllocated); byte[] decoded = await encoder.DecodeAsync(encoded, TestContext.CancellationToken).ConfigureAwait(false); CollectionAssert.AreEqual(original, decoded, $"{providerName} should decode what it encoded"); + + using MemoryStream fromMemoryDecoded = new(); + Assert.IsTrue( + await encoder.TryDecodeAsync(selfAllocated.AsMemory(), fromMemoryDecoded, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} async should decode from memory to a stream"); + CollectionAssert.AreEqual(original, fromMemoryDecoded.ToArray(), $"{providerName} overloads should agree on decoding"); } /// @@ -244,4 +250,135 @@ private sealed class DripStream(byte[] content) : MemoryStream(content) public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) => base.ReadAsync(buffer[..Math.Min(buffer.Length, 1)], cancellationToken); } + + /// + /// Tests that a null stream is reported rather than thrown, matching the synchronous paths. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_RejectsNullStreams(IEncodingProvider encoder, string providerName) + { + using MemoryStream real = new([1, 2, 3]); + + Assert.IsFalse(await encoder.TryEncodeAsync((Stream)null!, real, TestContext.CancellationToken).ConfigureAwait(false), providerName); + Assert.IsFalse(await encoder.TryEncodeAsync(real, null!, TestContext.CancellationToken).ConfigureAwait(false), providerName); + Assert.IsFalse(await encoder.TryDecodeAsync((Stream)null!, real, TestContext.CancellationToken).ConfigureAwait(false), providerName); + Assert.IsFalse(await encoder.TryDecodeAsync(real, null!, TestContext.CancellationToken).ConfigureAwait(false), providerName); + } + + /// + /// Tests that input the provider cannot decode is reported rather than producing garbage. + /// + /// + /// Both cases are malformed for both providers: a single character is neither an even number of + /// hex digits nor a whole Base64 quantum, and z is not a hex digit while ! is not a + /// Base64 character. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_ReportsMalformedInput(IEncodingProvider encoder, string providerName) + { + foreach (string malformed in MalformedInputs) + { + using MemoryStream input = new(Encoding.UTF8.GetBytes(malformed)); + using MemoryStream output = new(); + + Assert.IsFalse( + await encoder.TryDecodeAsync(input, output, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} should refuse '{malformed}'"); + } + } + + /// + /// Input neither provider can decode: a lone character is neither an even number of hex digits + /// nor a whole Base64 quantum, and the four-character case holds a character neither alphabet + /// contains. + /// + private static readonly string[] MalformedInputs = ["z", "zzz!"]; + + /// + /// Tests that a stream failing mid-operation is reported rather than thrown out of the provider. + /// + /// + /// A stream that faults part-way is ordinary — a dropped connection, a full disk — and these are + /// Try methods, so the failure belongs in the return value. Covers both the read side and + /// the write side, since they are separate awaits with separate failure modes. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_ReportsAFailingStream(IEncodingProvider encoder, string providerName) + { + byte[] payload = Encoding.UTF8.GetBytes("something to encode"); + + using (FailingStream unreadable = new()) + using (MemoryStream output = new()) + { + Assert.IsFalse( + await encoder.TryEncodeAsync(unreadable, output, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} should report a source that fails to read"); + } + + using (MemoryStream input = new(payload)) + using (FailingStream unwritable = new()) + { + Assert.IsFalse( + await encoder.TryEncodeAsync(input, unwritable, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} should report a destination that fails to write"); + } + } + + /// + /// Tests that a stream disposed before use is reported rather than thrown. + /// + [TestMethod] + [DynamicData(nameof(EncodingProviders))] + public async Task Encoding_Async_ReportsADisposedStream(IEncodingProvider encoder, string providerName) + { + MemoryStream disposed = new([1, 2, 3, 4]); + await disposed.DisposeAsync().ConfigureAwait(false); + + using MemoryStream output = new(); + + Assert.IsFalse( + await encoder.TryEncodeAsync(disposed, output, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} should report a disposed source"); + } + + /// + /// A stream that fails whichever way it is used. + /// + private sealed class FailingStream : Stream + { + public override bool CanRead => true; + + public override bool CanSeek => false; + + public override bool CanWrite => true; + + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override int Read(byte[] buffer, int offset, int count) => throw new IOException("read failed"); + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + => throw new IOException("read failed"); + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new IOException("write failed"); + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + => throw new IOException("write failed"); + } } From 7e84ec8dbf990af54ed37ed7ff810d17796c8ffb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 12:16:52 +0000 Subject: [PATCH 3/4] fix: dispose the test's stream deterministically [patch] `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 Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg --- Essentials.Tests/EncodingProviderTests.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/Essentials.Tests/EncodingProviderTests.cs b/Essentials.Tests/EncodingProviderTests.cs index cdefc36..7e6ca85 100644 --- a/Essentials.Tests/EncodingProviderTests.cs +++ b/Essentials.Tests/EncodingProviderTests.cs @@ -334,14 +334,20 @@ await encoder.TryEncodeAsync(input, unwritable, TestContext.CancellationToken).C [DynamicData(nameof(EncodingProviders))] public async Task Encoding_Async_ReportsADisposedStream(IEncodingProvider encoder, string providerName) { + // Disposed twice on purpose: the explicit call is the state under test, and the enclosing + // declaration guarantees disposal even if the assertion throws first. MemoryStream.Dispose is + // idempotent, so the second call is a no-op. MemoryStream disposed = new([1, 2, 3, 4]); - await disposed.DisposeAsync().ConfigureAwait(false); + await using (disposed.ConfigureAwait(false)) + { + await disposed.DisposeAsync().ConfigureAwait(false); - using MemoryStream output = new(); + using MemoryStream output = new(); - Assert.IsFalse( - await encoder.TryEncodeAsync(disposed, output, TestContext.CancellationToken).ConfigureAwait(false), - $"{providerName} should report a disposed source"); + Assert.IsFalse( + await encoder.TryEncodeAsync(disposed, output, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} should report a disposed source"); + } } /// From ff56392a48a11469c0abcd39696b0afd724acf42 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 12:22:10 +0000 Subject: [PATCH 4/4] refactor: move the disposed-stream setup out of the test body [patch] 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 Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg --- Essentials.Tests/EncodingProviderTests.cs | 33 ++++++++++++++--------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/Essentials.Tests/EncodingProviderTests.cs b/Essentials.Tests/EncodingProviderTests.cs index 7e6ca85..ba5f755 100644 --- a/Essentials.Tests/EncodingProviderTests.cs +++ b/Essentials.Tests/EncodingProviderTests.cs @@ -334,20 +334,29 @@ await encoder.TryEncodeAsync(input, unwritable, TestContext.CancellationToken).C [DynamicData(nameof(EncodingProviders))] public async Task Encoding_Async_ReportsADisposedStream(IEncodingProvider encoder, string providerName) { - // Disposed twice on purpose: the explicit call is the state under test, and the enclosing - // declaration guarantees disposal even if the assertion throws first. MemoryStream.Dispose is - // idempotent, so the second call is a no-op. - MemoryStream disposed = new([1, 2, 3, 4]); - await using (disposed.ConfigureAwait(false)) - { - await disposed.DisposeAsync().ConfigureAwait(false); + MemoryStream disposed = await ClosedStreamAsync().ConfigureAwait(false); + using MemoryStream output = new(); - using MemoryStream output = new(); + Assert.IsFalse( + await encoder.TryEncodeAsync(disposed, output, TestContext.CancellationToken).ConfigureAwait(false), + $"{providerName} should report a disposed source"); + } - Assert.IsFalse( - await encoder.TryEncodeAsync(disposed, output, TestContext.CancellationToken).ConfigureAwait(false), - $"{providerName} should report a disposed source"); - } + /// + /// Builds a stream that has already been closed. + /// + /// A disposed stream. + /// + /// The disposal happens here rather than in the test body so that nothing the test holds is an + /// undisposed resource: a caller receives an object whose lifetime is already over and owns + /// nothing. Expressing it inline is what turns awkward — a disposed local still looks like a leak + /// to disposal analysis, and the shapes that convince it collide with the rules about awaiting. + /// + private static async Task ClosedStreamAsync() + { + MemoryStream stream = new([1, 2, 3, 4]); + await stream.DisposeAsync().ConfigureAwait(false); + return stream; } ///