From 8097779b828bed9a9f8e101c46e12f4c6d732a09 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 31 Aug 2026 22:07:54 -0700 Subject: [PATCH 01/11] Make the Download Tests Offline and Tighten the StringHistory Limit Contract Closes the three open issues on this repository. #454, StringHistory limit properties. MaxFirstLines and MaxLastLines were each documented "Set to 0 for no limit", which holds only when both are zero, the one branch AppendLine treats as unrestricted. Set individually, zero retains none on that side, so the documentation now says that. Both the constructor and the two setters reject a negative value with ArgumentOutOfRangeException, rather than letting it reach RemoveAt and throw after entries have already been taken. Each setter re-partitions the stored entries and resets the counters, so a limit assigned after appending is honored instead of ignored. ARCHITECTURE.md "Public API Conventions" records the contract, and README.md and HISTORY.md carry the behavioral change. #452, download tests. DownloadTests and DownloadAsyncTests reached real hosts, so the unit-test gate depended on outbound DNS and on two third parties. They now drive a loopback HttpListener bound to an ephemeral port, which makes success, not-found, and cancellation deterministic and offline, and drops the five-second sleep the timeout case paid on every run. The negative case is a 404 from that server rather than a name expected never to resolve, which a wildcard or a captive-portal resolver would have silently inverted. Download's public surface is unchanged, so the Lazy contract in ARCHITECTURE.md still holds. #427, coordination reference. HISTORY.md named the template repository in its v3.5 entry. The entry now states what changed without naming where it came from. .github/copilot-instructions.md was already clear. --- ARCHITECTURE.md | 1 + HISTORY.md | 3 +- README.md | 1 + Utilities/StringHistory.cs | 82 +++++++++++- UtilitiesTests/DownloadAsyncTests.cs | 37 +++--- UtilitiesTests/DownloadTests.cs | 19 ++- UtilitiesTests/LoopbackServer.cs | 184 +++++++++++++++++++++++++++ UtilitiesTests/StringHistoryTests.cs | 139 ++++++++++++++++++++ 8 files changed, 433 insertions(+), 33 deletions(-) create mode 100644 UtilitiesTests/LoopbackServer.cs diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index f54f9b5..3a40264 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -21,5 +21,6 @@ These are behavioral contracts rather than formatting rules, which is why they l - **`Download`** reuses a thread-safe `Lazy` and reads with `HttpCompletionOption.ResponseHeadersRead`, so a large response streams rather than buffering whole. - **`FileEx`** wraps its I/O in retry logic configured through `Options`, and honors cancellation from both `Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate, takes a configurable compression level, and passes `leaveOpen` so the caller keeps ownership of the stream it supplied. +- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Both limits at zero is the one unrestricted mode, and zero on a single side retains no lines on that side. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. - **`Extensions`** uses the C# `extension` block form inside a static class for its logger and string helpers. - **Logging is a seam, never a dependency.** The library depends on `Microsoft.Extensions.Logging.Abstractions` and takes an `ILoggerFactory` through `LogOptions`. It references no logging framework or sink, so a consumer chooses its own. `Serilog` appears only in `Sandbox` and the tests, where an application legitimately picks one. diff --git a/HISTORY.md b/HISTORY.md index 91afb9f..a8f82b7 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,6 +5,7 @@ Some useful and not so useful C# .NET utility classes. ## Release History - v4.0: + - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later append, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`; it exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. - Added `AssemblyInfo`, an AOT safe assembly and application identity helper whose `For()` substitutes for `Assembly.GetExecutingAssembly()` (unreliable under Native AOT), and which supplies the consuming application name, version, and a default User-Agent. - Reworked `Download` to build its `HttpClient` through `HttpClientFactory`, so downloads now flow through the shared retry and circuit-breaker pipeline; the `TimeoutSeconds` property and all method signatures are unchanged. @@ -24,7 +25,7 @@ Some useful and not so useful C# .NET utility classes. - Added `WORKFLOW.md`, the canonical CI/CD specification, and `repo-config/`, the rulesets and repository settings as code. - Bundled the build output and NuGet packages into a `Utilities.7z` asset on each GitHub release. - v3.5: - - Re-synced the repository structure and agent documentation with the upstream ProjectTemplate: added this `HISTORY.md` and a `CODESTYLE.md` .NET style guide, narrowed `.github/copilot-instructions.md` to the Copilot review runbook, and refreshed `AGENTS.md` conventions. + - Re-synced the repository structure and agent documentation: added this `HISTORY.md` and a `CODESTYLE.md` .NET style guide, narrowed `.github/copilot-instructions.md` to the Copilot review runbook, and refreshed `AGENTS.md` conventions. - Corrected the versioning policy to bump `version.json` only for functional changes. - Swapped the recommended Todo VS Code add-on from Todo Tree to Better Todo Tree. - v3.4: diff --git a/README.md b/README.md index 4602ce0..ebdb48b 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ Some useful and not so useful C# .NET utility classes. - Renamed the NuGet package and namespace from `InsaneGenius.Utilities` to `ptr727.Utilities`. - Replaced Serilog with an injectable `Microsoft.Extensions.Logging` logging model. - Replaced `List` in public methods with `Collection` and `ReadOnlyCollection`. +- Changed `StringHistory.MaxFirstLines` and `MaxLastLines` to re-partition the lines already stored when assigned, and to reject a negative value. **Summary**: diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index e42a551..4460485 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -5,7 +5,10 @@ namespace ptr727.Utilities; /// /// /// This class is useful for maintaining a bounded history buffer, keeping the first N and last M lines -/// while discarding intermediate content when limits are exceeded. +/// while discarding intermediate content when limits are exceeded. Both limits at zero is the one +/// unrestricted mode, where every appended line is retained; zero on a single side retains no lines +/// on that side. A limit assigned after lines have been appended re-partitions what is already +/// stored, so the history never holds more than the limits then in force allow. /// public class StringHistory { @@ -17,11 +20,18 @@ public class StringHistory /// /// Initializes a new instance of the class with specified limits. /// - /// Maximum number of first lines to retain. - /// Maximum number of last lines to retain. + /// Maximum number of first lines to retain, or 0 to retain none. + /// Maximum number of last lines to retain, or 0 to retain none. + /// + /// Thrown when or is negative. + /// public StringHistory(int maxFirstLines, int maxLastLines) : this() { + // Validated here rather than in the setters so the exception names the caller's parameter. + ArgumentOutOfRangeException.ThrowIfNegative(maxFirstLines); + ArgumentOutOfRangeException.ThrowIfNegative(maxLastLines); + MaxFirstLines = maxFirstLines; MaxLastLines = maxLastLines; } @@ -79,22 +89,80 @@ public override string ToString() => /// /// Gets or sets the maximum number of first lines to retain. - /// Set to 0 for no limit. + /// Set to 0 to retain no first lines; both limits at 0 retains every line. /// - public int MaxFirstLines { get; set; } + /// + /// Assigning this re-partitions the lines already stored against the limits then in force, + /// which discards whatever the new limits exclude. Setting both limits therefore applies them + /// one at a time, and the first assignment can discard lines the second would have retained. + /// Prefer the two-argument constructor when both limits are known up front. + /// + /// Thrown when the assigned value is negative. + public int MaxFirstLines + { + get => _maxFirstLines; + set + { + ArgumentOutOfRangeException.ThrowIfNegative(value); + _maxFirstLines = value; + Repartition(); + } + } /// /// Gets or sets the maximum number of last lines to retain. - /// Set to 0 for no limit. + /// Set to 0 to retain no last lines; both limits at 0 retains every line. /// - public int MaxLastLines { get; set; } + /// + /// Assigning this re-partitions the lines already stored against the limits then in force, + /// which discards whatever the new limits exclude. Setting both limits therefore applies them + /// one at a time, and the first assignment can discard lines the second would have retained. + /// Prefer the two-argument constructor when both limits are known up front. + /// + /// Thrown when the assigned value is negative. + public int MaxLastLines + { + get => _maxLastLines; + set + { + ArgumentOutOfRangeException.ThrowIfNegative(value); + _maxLastLines = value; + Repartition(); + } + } /// /// Gets the read-only collection of stored strings. /// public ReadOnlyCollection StringList { get; } + /// + /// Brings the stored lines within the current limits and resets the counters to match, so an + /// append after a limit change continues from what is actually stored. + /// + private void Repartition() + { + // Both limits at zero is the unrestricted mode, where every line is retained. + if (MaxFirstLines == 0 && MaxLastLines == 0) + { + _firstLines = 0; + _lastLines = 0; + return; + } + + int firstLines = Math.Min(MaxFirstLines, _stringList.Count); + int lastLines = Math.Min(MaxLastLines, _stringList.Count - firstLines); + + // Drop the lines between the retained head and the retained tail. + _stringList.RemoveRange(firstLines, _stringList.Count - firstLines - lastLines); + + _firstLines = firstLines; + _lastLines = lastLines; + } + private readonly List _stringList = []; + private int _maxFirstLines; + private int _maxLastLines; private int _firstLines; private int _lastLines; } diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index 5791d60..328fc7e 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -5,53 +5,48 @@ public class DownloadAsyncTests [Fact] public async Task GetContentInfoAsync_WithValidUri_ShouldReturnSuccess() { - Uri uri = new( - "https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png" - ); + using LoopbackServer server = new(); (bool success, long size, DateTime _) = await Download.GetContentInfoAsync( - uri, + server.OkUri, TestContext.Current.CancellationToken ); _ = success.Should().BeTrue(); - _ = (size > 0).Should().BeTrue(); + _ = size.Should().Be(LoopbackServer.Content.Length); } [Fact] public async Task DownloadStringAsync_WithValidUri_ShouldReturnContent() { - Uri uri = new("https://www.google.com"); + using LoopbackServer server = new(); (bool success, string? content) = await Download.DownloadStringAsync( - uri, + server.OkUri, TestContext.Current.CancellationToken ); _ = success.Should().BeTrue(); - _ = content.Should().NotBeEmpty(); - _ = content.Should().ContainEquivalentOf("google"); + _ = content.Should().Be(LoopbackServer.Content); } [Fact] public async Task DownloadFileAsync_WithValidUri_ShouldCreateFile() { - Uri uri = new( - "https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png" - ); + using LoopbackServer server = new(); string tempFile = Path.GetTempFileName(); try { bool result = await Download.DownloadFileAsync( - uri, + server.OkUri, tempFile, TestContext.Current.CancellationToken ); _ = result.Should().BeTrue(); _ = File.Exists(tempFile).Should().BeTrue(); - _ = (new FileInfo(tempFile).Length > 0).Should().BeTrue(); + _ = new FileInfo(tempFile).Length.Should().Be(LoopbackServer.Content.Length); } finally { @@ -63,12 +58,12 @@ public async Task DownloadFileAsync_WithValidUri_ShouldCreateFile() } [Fact] - public async Task DownloadAsync_WithInvalidUri_ShouldReturnFalse() + public async Task GetContentInfoAsync_WithNotFoundUri_ShouldReturnFalse() { - Uri invalidUri = new("https://thisdoesnotexist123456789.com/file.txt"); + using LoopbackServer server = new(); (bool success, long _, DateTime _) = await Download.GetContentInfoAsync( - invalidUri, + server.MissingUri, TestContext.Current.CancellationToken ); @@ -78,13 +73,13 @@ public async Task DownloadAsync_WithInvalidUri_ShouldReturnFalse() [Fact] public async Task DownloadAsync_WithCancellation_ShouldRespectCancellation() { - Uri uri = new("https://httpstat.us/200?sleep=5000"); // Slow endpoint + using LoopbackServer server = new(); using CancellationTokenSource cts = new(); - cts.CancelAfter(TimeSpan.FromMilliseconds(100)); // Cancel after 100ms + cts.CancelAfter(TimeSpan.FromMilliseconds(100)); - (bool Success, string _) = await Download.DownloadStringAsync(uri, cts.Token); + // The slow route outlasts the cancellation delay, so the token always wins the race. + (bool Success, string _) = await Download.DownloadStringAsync(server.SlowUri, cts.Token); - // Should either throw cancellation or return false due to cancellation _ = Success.Should().BeFalse(); } diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index 75bd0da..4b36f69 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -5,9 +5,20 @@ public class DownloadTests [Fact] public void GetUriInformation() { - Uri uri = new( - "https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png" - ); - _ = Download.GetContentInfo(uri, out long _, out DateTime _).Should().BeTrue(); + using LoopbackServer server = new(); + + _ = Download.GetContentInfo(server.OkUri, out long size, out DateTime _).Should().BeTrue(); + _ = size.Should().Be(LoopbackServer.Content.Length); + } + + [Fact] + public void GetContentInfo_WithNotFoundUri_ShouldReturnFalse() + { + using LoopbackServer server = new(); + + _ = Download + .GetContentInfo(server.MissingUri, out long _, out DateTime _) + .Should() + .BeFalse(); } } diff --git a/UtilitiesTests/LoopbackServer.cs b/UtilitiesTests/LoopbackServer.cs new file mode 100644 index 0000000..961e434 --- /dev/null +++ b/UtilitiesTests/LoopbackServer.cs @@ -0,0 +1,184 @@ +using System.Net; +using System.Net.Sockets; +using System.Text; + +namespace ptr727.Utilities.Tests; + +/// +/// A minimal HTTP server bound to the loopback interface, so the tests +/// exercise success, failure, and cancellation without reaching the network or depending on a +/// third party staying reachable. +/// +/// +/// Each instance binds its own ephemeral port, so tests running in parallel do not share routes or +/// state. Disposing the server stops the listener and completes any request still in flight. +/// +internal sealed class LoopbackServer : IDisposable +{ + /// + /// The body every successful route returns. + /// + public const string Content = "loopback content"; + + /// + /// How long the slow route holds a request before responding, long enough that a caller + /// cancelling after a short delay wins the race deterministically. + /// + public static readonly TimeSpan SlowResponseDelay = TimeSpan.FromSeconds(30); + + /// + /// Initializes a new instance of the class and starts listening. + /// + public LoopbackServer() + { + // HttpListener takes a prefix rather than a socket, so a free port is probed and released first. + BaseAddress = new Uri( + FormattableString.Invariant($"http://127.0.0.1:{GetEphemeralPort()}/") + ); + _listener.Prefixes.Add(BaseAddress.AbsoluteUri); + _listener.Start(); + _listenTask = Task.Run(ListenAsync); + } + + /// + /// Gets the root address the server is listening on. + /// + public Uri BaseAddress { get; } + + /// + /// Gets the address of the route that responds 200 with . + /// + public Uri OkUri => new(BaseAddress, OkPath); + + /// + /// Gets the address of the route that responds 404. + /// + public Uri MissingUri => new(BaseAddress, MissingPath); + + /// + /// Gets the address of the route that waits before responding. + /// + public Uri SlowUri => new(BaseAddress, SlowPath); + + /// + /// Stops the listener and waits for the accept loop to finish. + /// + public void Dispose() + { + _cancellation.Cancel(); + _listener.Close(); + + // The accept loop swallows its own failures, so a stopped listener is not a teardown failure. + _listenTask.GetAwaiter().GetResult(); + + _cancellation.Dispose(); + } + + private static int GetEphemeralPort() + { + using TcpListener probe = new(IPAddress.Loopback, 0); + probe.Start(); + return ((IPEndPoint)probe.LocalEndpoint).Port; + } + + private async Task ListenAsync() + { + while (!_cancellation.IsCancellationRequested) + { + HttpListenerContext context; + try + { + context = await _listener.GetContextAsync().ConfigureAwait(false); + } + catch (HttpListenerException) + { + // The listener was closed by Dispose while waiting for a request. + return; + } + catch (ObjectDisposedException) + { + return; + } + + await RespondAsync(context).ConfigureAwait(false); + } + } + + private async Task RespondAsync(HttpListenerContext context) + { + HttpListenerResponse response = context.Response; + try + { + switch (context.Request.Url?.AbsolutePath) + { + case "/" + OkPath: + await WriteContentAsync(response).ConfigureAwait(false); + break; + + case "/" + SlowPath: + await Task.Delay(SlowResponseDelay, _cancellation.Token).ConfigureAwait(false); + await WriteContentAsync(response).ConfigureAwait(false); + break; + + default: + response.StatusCode = (int)HttpStatusCode.NotFound; + break; + } + } + catch (OperationCanceledException) + { + // Dispose ran, or the client went away, while the slow route was waiting. + } + catch (HttpListenerException) + { + // The client disconnected before the response was written, as the cancellation test does. + } + catch (IOException) + { + // Same disconnect, surfaced from the response stream rather than the listener. + } + finally + { + Close(response); + } + } + + private static async Task WriteContentAsync(HttpListenerResponse response) + { + byte[] body = Encoding.UTF8.GetBytes(Content); + + // GetContentInfo reports Content-Length as the size, so it is set rather than left to chunking. + response.StatusCode = (int)HttpStatusCode.OK; + response.ContentType = "text/plain; charset=utf-8"; + response.ContentLength64 = body.Length; + response.Headers.Set("Last-Modified", s_lastModified.ToString("R", null)); + + await response.OutputStream.WriteAsync(body).ConfigureAwait(false); + } + + private static void Close(HttpListenerResponse response) + { + try + { + response.Close(); + } + catch (HttpListenerException) + { + // Closing a response whose client already disconnected is not a failure. + } + catch (ObjectDisposedException) + { + // The listener was disposed first. + } + } + + private const string OkPath = "ok"; + private const string MissingPath = "missing"; + private const string SlowPath = "slow"; + + private static readonly DateTimeOffset s_lastModified = new(2020, 1, 1, 0, 0, 0, TimeSpan.Zero); + + private readonly HttpListener _listener = new(); + private readonly CancellationTokenSource _cancellation = new(); + private readonly Task _listenTask; +} diff --git a/UtilitiesTests/StringHistoryTests.cs b/UtilitiesTests/StringHistoryTests.cs index 3efa4c8..cb95ba0 100644 --- a/UtilitiesTests/StringHistoryTests.cs +++ b/UtilitiesTests/StringHistoryTests.cs @@ -228,4 +228,143 @@ public void AppendLine_UnicodeCharacters_ShouldPreserve() _ = history.StringList[0].Should().Be(unicodeLine); } + + [Fact] + public void Constructor_WithNegativeFirstLines_ShouldThrowArgumentOutOfRangeException() + { + _ = FluentActions + .Invoking(() => new StringHistory(maxFirstLines: -1, maxLastLines: 2)) + .Should() + .Throw() + .WithParameterName("maxFirstLines"); + } + + [Fact] + public void Constructor_WithNegativeLastLines_ShouldThrowArgumentOutOfRangeException() + { + _ = FluentActions + .Invoking(() => new StringHistory(maxFirstLines: 2, maxLastLines: -1)) + .Should() + .Throw() + .WithParameterName("maxLastLines"); + } + + [Fact] + public void MaxFirstLines_SetNegative_ShouldThrowArgumentOutOfRangeException() + { + StringHistory history = new(); + + _ = FluentActions + .Invoking(() => history.MaxFirstLines = -1) + .Should() + .Throw(); + + // The rejected value never reaches the property. + _ = history.MaxFirstLines.Should().Be(0); + } + + [Fact] + public void MaxLastLines_SetNegative_ShouldThrowArgumentOutOfRangeException() + { + StringHistory history = new(); + + _ = FluentActions + .Invoking(() => history.MaxLastLines = -1) + .Should() + .Throw(); + + _ = history.MaxLastLines.Should().Be(0); + } + + [Fact] + public void MaxLastLines_SetAfterAppend_ShouldRepartitionStoredLines() + { + StringHistory history = new(); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + history.MaxLastLines = 3; + + // The unbounded lines already stored are brought within the new limit rather than kept. + _ = history.StringList.Count.Should().Be(3); + _ = history.StringList[0].Should().Be("Line 7"); + _ = history.StringList[2].Should().Be("Line 9"); + } + + [Fact] + public void MaxFirstLines_SetAfterAppend_ShouldRepartitionStoredLines() + { + StringHistory history = new(); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + history.MaxFirstLines = 2; + + // MaxLastLines is still 0, which retains no last lines, so only the head survives. + _ = history.StringList.Count.Should().Be(2); + _ = history.StringList[0].Should().Be("Line 0"); + _ = history.StringList[1].Should().Be("Line 1"); + } + + [Fact] + public void AppendLine_AfterLimitChange_ShouldHonorTheNewLimits() + { + StringHistory history = new(); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + history.MaxLastLines = 2; + history.AppendLine("Line 10"); + + // Appending rolls the retained tail rather than growing past the configured bound. + _ = history.StringList.Count.Should().Be(2); + _ = history.StringList[0].Should().Be("Line 9"); + _ = history.StringList[1].Should().Be("Line 10"); + } + + [Fact] + public void MaxFirstLines_ReducedBelowStoredCount_ShouldTrimTheHead() + { + StringHistory history = new(maxFirstLines: 5, maxLastLines: 2); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + history.MaxFirstLines = 2; + + // 2 head lines plus the 2 tail lines the previous limits already retained. + _ = history.StringList.Count.Should().Be(4); + _ = history.StringList[0].Should().Be("Line 0"); + _ = history.StringList[1].Should().Be("Line 1"); + _ = history.StringList[2].Should().Be("Line 8"); + _ = history.StringList[3].Should().Be("Line 9"); + } + + [Fact] + public void Limits_ClearedToZero_ShouldRetainEveryLaterLine() + { + StringHistory history = new(maxFirstLines: 2, maxLastLines: 2); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + history.MaxFirstLines = 0; + history.MaxLastLines = 0; + history.AppendLine("Line 10"); + + // Clearing MaxFirstLines first re-partitions against a MaxLastLines still at 2, leaving the tail. + // Clearing MaxLastLines then restores the unrestricted mode, so the later line is added. + _ = history.StringList.Count.Should().Be(3); + _ = history.StringList[0].Should().Be("Line 8"); + _ = history.StringList[1].Should().Be("Line 9"); + _ = history.StringList[2].Should().Be("Line 10"); + } } From ef93bf551da330a9dac3c9e4e0fda5fbf7421bfb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 31 Aug 2026 22:33:26 -0700 Subject: [PATCH 02/11] Close the Defects the Local Review Found A local adversarial pass over the previous commit raised seven findings against it. Six are fixed here and one is declined. StringHistory. Raising MaxFirstLines on a history that had already dropped a line promoted retained tail lines into the head, where they could never roll out again, so [L0,L1,L8,L9] with the limit raised to 5 kept L8 and L9 as first lines forever. That contradicted the head and tail contract this change had just written into ARCHITECTURE.md. The head is now closed once anything has been discarded: it only shrinks from there, re-partitioning trims the head and the tail separately rather than re-slicing a list that has a gap in it, and the roll indexes the stored head rather than the limit, which a widened limit would otherwise index past the end of the list. Three tests cover the closed head, the still-open head, and the head-only case, and a 200000-history fuzz over random append and limit sequences held the ordering, bound, and no-duplicate invariants across three million assertions. LoopbackServer. Rewritten onto a TcpListener rather than HttpListener, which fixes three findings at once. Binding port 0 and reading the port back removes the probe-then-release window that failed 6 constructions in 4800 concurrent ones. Writing the response onto the socket removes the http.sys URL reservation that an explicit-address prefix needs on Windows, where GOVERNANCE.md declares Windows a supported development platform and CI is Linux-only. Serving the connection with a catch-all removes the unexpected exception that escaped the accept loop, faulted the task Dispose waits on, and reddened a test whose assertions had all passed. Dispose is also idempotent now and its wait is bounded, and the fixture reports the content's byte length rather than its UTF-16 char count. Cancellation test. It asserted only that the call returned false, which any failure produces, so it could not tell a cancelled request from a server that never started. It now waits for the server to signal that the slow route has the request, cancels only then, and asserts the call returned well inside the route's own delay. Proxy independence. HttpClientFactory leaves SocketsHttpHandler.UseProxy at its default, so a client resolves HttpClient.DefaultProxy, which on Unix reads http_proxy and bypasses loopback only where no_proxy says to. A machine configured that way would send these loopback requests to a proxy, which is the environment dependence #452 exists to remove, so the test assembly disables the ambient proxy. Declined: the finding that the HISTORY.md v3.5 entry is an out-of-scope edit to a shipped release record. That edit is #427's deliverable, not incidental churn. Not fixed here, and raised for a separate decision: Download.DownloadFile and DownloadFileAsync open the destination with File.OpenWrite, which does not truncate, so downloading over a longer existing file leaves trailing bytes from the previous content. It is a pre-existing defect in shipped code, outside all three issues this branch closes. --- ARCHITECTURE.md | 2 +- Utilities/StringHistory.cs | 60 ++++++-- UtilitiesTests/DownloadAsyncTests.cs | 32 ++++- UtilitiesTests/DownloadTests.cs | 2 +- UtilitiesTests/LoopbackServer.cs | 196 +++++++++++++++++---------- UtilitiesTests/StringHistoryTests.cs | 57 ++++++++ UtilitiesTests/TestHttpDefaults.cs | 24 ++++ 7 files changed, 281 insertions(+), 92 deletions(-) create mode 100644 UtilitiesTests/TestHttpDefaults.cs diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 3a40264..60bb31d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -21,6 +21,6 @@ These are behavioral contracts rather than formatting rules, which is why they l - **`Download`** reuses a thread-safe `Lazy` and reads with `HttpCompletionOption.ResponseHeadersRead`, so a large response streams rather than buffering whole. - **`FileEx`** wraps its I/O in retry logic configured through `Options`, and honors cancellation from both `Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate, takes a configurable compression level, and passes `leaveOpen` so the caller keeps ownership of the stream it supplied. -- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Both limits at zero is the one unrestricted mode, and zero on a single side retains no lines on that side. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. +- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Both limits at zero is the one unrestricted mode, and zero on a single side retains no lines on that side. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. - **`Extensions`** uses the C# `extension` block form inside a static class for its logger and string helpers. - **Logging is a seam, never a dependency.** The library depends on `Microsoft.Extensions.Logging.Abstractions` and takes an `ILoggerFactory` through `LogOptions`. It references no logging framework or sink, so a consumer chooses its own. `Serilog` appears only in `Sandbox` and the tests, where an application legitimately picks one. diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index 4460485..f060e8d 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -8,7 +8,9 @@ namespace ptr727.Utilities; /// while discarding intermediate content when limits are exceeded. Both limits at zero is the one /// unrestricted mode, where every appended line is retained; zero on a single side retains no lines /// on that side. A limit assigned after lines have been appended re-partitions what is already -/// stored, so the history never holds more than the limits then in force allow. +/// stored, so the history never holds more than the limits then in force allow. Re-partitioning +/// only ever discards: once a line has been dropped the head is closed, and a later, larger +/// raises the ceiling without recovering or repopulating it. /// public class StringHistory { @@ -49,11 +51,14 @@ public void AppendLine(string value) if (MaxFirstLines == 0 && MaxLastLines == 0) { _stringList.Add(value); + _firstLines++; return; } // Restrict first lines - if (_firstLines < MaxFirstLines) + // The head only grows while nothing has been discarded. + // A line appended once a tail exists is stored behind it and is not a first line. + if (!_discarded && _firstLines < MaxFirstLines) { _stringList.Add(value); _firstLines++; @@ -63,6 +68,7 @@ public void AppendLine(string value) // If MaxLastLines is 0, don't add any more lines after MaxFirstLines if (MaxLastLines == 0) { + _discarded = true; return; } @@ -75,8 +81,10 @@ public void AppendLine(string value) } // Roll the last lines - _stringList.RemoveAt(MaxFirstLines); + // The head is what is stored rather than the limit, which a widened one would index past. + _stringList.RemoveAt(_firstLines); _stringList.Add(value); + _discarded = true; } /// @@ -93,9 +101,10 @@ public override string ToString() => /// /// /// Assigning this re-partitions the lines already stored against the limits then in force, - /// which discards whatever the new limits exclude. Setting both limits therefore applies them - /// one at a time, and the first assignment can discard lines the second would have retained. - /// Prefer the two-argument constructor when both limits are known up front. + /// which discards whatever the new limits exclude and never recovers a line already dropped. + /// Setting both limits therefore applies them one at a time, and the first assignment can + /// discard lines the second would have retained. Prefer the two-argument constructor when both + /// limits are known up front. /// /// Thrown when the assigned value is negative. public int MaxFirstLines @@ -115,9 +124,10 @@ public int MaxFirstLines /// /// /// Assigning this re-partitions the lines already stored against the limits then in force, - /// which discards whatever the new limits exclude. Setting both limits therefore applies them - /// one at a time, and the first assignment can discard lines the second would have retained. - /// Prefer the two-argument constructor when both limits are known up front. + /// which discards whatever the new limits exclude and never recovers a line already dropped. + /// Setting both limits therefore applies them one at a time, and the first assignment can + /// discard lines the second would have retained. Prefer the two-argument constructor when both + /// limits are known up front. /// /// Thrown when the assigned value is negative. public int MaxLastLines @@ -143,21 +153,42 @@ public int MaxLastLines private void Repartition() { // Both limits at zero is the unrestricted mode, where every line is retained. + // Nothing is discarded there, so the whole list is head and a later limit slices it freely. if (MaxFirstLines == 0 && MaxLastLines == 0) { - _firstLines = 0; + _firstLines = _stringList.Count; _lastLines = 0; return; } - int firstLines = Math.Min(MaxFirstLines, _stringList.Count); - int lastLines = Math.Min(MaxLastLines, _stringList.Count - firstLines); + int storedCount = _stringList.Count; + int firstLines; + int lastLines; - // Drop the lines between the retained head and the retained tail. - _stringList.RemoveRange(firstLines, _stringList.Count - firstLines - lastLines); + if (_discarded) + { + // A gap already separates the head from the tail, so the head can only shrink. + // Promoting a tail line into it would pin a line that is not among the first. + firstLines = Math.Min(MaxFirstLines, _firstLines); + lastLines = Math.Min(MaxLastLines, _lastLines); + _stringList.RemoveRange(firstLines, _firstLines - firstLines); + _stringList.RemoveRange(firstLines, _lastLines - lastLines); + } + else + { + // Nothing has been discarded, so the stored lines are the whole stream so far. + // The new head and the new tail are both cut from them. + firstLines = Math.Min(MaxFirstLines, storedCount); + lastLines = Math.Min(MaxLastLines, storedCount - firstLines); + _stringList.RemoveRange(firstLines, storedCount - firstLines - lastLines); + } _firstLines = firstLines; _lastLines = lastLines; + if (_stringList.Count < storedCount) + { + _discarded = true; + } } private readonly List _stringList = []; @@ -165,4 +196,5 @@ private void Repartition() private int _maxLastLines; private int _firstLines; private int _lastLines; + private bool _discarded; } diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index 328fc7e..e1576f0 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -1,3 +1,5 @@ +using System.Diagnostics; + namespace ptr727.Utilities.Tests; public class DownloadAsyncTests @@ -13,7 +15,7 @@ public async Task GetContentInfoAsync_WithValidUri_ShouldReturnSuccess() ); _ = success.Should().BeTrue(); - _ = size.Should().Be(LoopbackServer.Content.Length); + _ = size.Should().Be(LoopbackServer.ContentLength); } [Fact] @@ -46,7 +48,7 @@ public async Task DownloadFileAsync_WithValidUri_ShouldCreateFile() _ = result.Should().BeTrue(); _ = File.Exists(tempFile).Should().BeTrue(); - _ = new FileInfo(tempFile).Length.Should().Be(LoopbackServer.Content.Length); + _ = new FileInfo(tempFile).Length.Should().Be(LoopbackServer.ContentLength); } finally { @@ -70,17 +72,35 @@ public async Task GetContentInfoAsync_WithNotFoundUri_ShouldReturnFalse() _ = success.Should().BeFalse(); } + [System.Diagnostics.CodeAnalysis.SuppressMessage( + "Reliability", + "CA2007:Consider calling ConfigureAwait on the awaited task", + Justification = "xUnit1030 forbids ConfigureAwait in a test method, and the request has to be started before it is cancelled." + )] [Fact] public async Task DownloadAsync_WithCancellation_ShouldRespectCancellation() { using LoopbackServer server = new(); using CancellationTokenSource cts = new(); - cts.CancelAfter(TimeSpan.FromMilliseconds(100)); - // The slow route outlasts the cancellation delay, so the token always wins the race. - (bool Success, string _) = await Download.DownloadStringAsync(server.SlowUri, cts.Token); + Task<(bool Success, string Value)> download = Download.DownloadStringAsync( + server.SlowUri, + cts.Token + ); + + // Cancelling only once the server holds the request proves the route was reached. + // Any other failure would produce the same false result and prove nothing. + await server.SlowRequestStarted.WaitAsync(TestContext.Current.CancellationToken); + long startedAt = Stopwatch.GetTimestamp(); + await cts.CancelAsync(); + + (bool success, string _) = await download; + TimeSpan elapsed = Stopwatch.GetElapsedTime(startedAt); + + _ = success.Should().BeFalse(); - _ = Success.Should().BeFalse(); + // Returning far inside the route's own delay separates a cancelled request from a served one. + _ = elapsed.Should().BeLessThan(LoopbackServer.SlowResponseDelay / 2); } [Fact] diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index 4b36f69..45cf137 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -8,7 +8,7 @@ public void GetUriInformation() using LoopbackServer server = new(); _ = Download.GetContentInfo(server.OkUri, out long size, out DateTime _).Should().BeTrue(); - _ = size.Should().Be(LoopbackServer.Content.Length); + _ = size.Should().Be(LoopbackServer.ContentLength); } [Fact] diff --git a/UtilitiesTests/LoopbackServer.cs b/UtilitiesTests/LoopbackServer.cs index 961e434..3ee9579 100644 --- a/UtilitiesTests/LoopbackServer.cs +++ b/UtilitiesTests/LoopbackServer.cs @@ -10,8 +10,10 @@ namespace ptr727.Utilities.Tests; /// third party staying reachable. /// /// -/// Each instance binds its own ephemeral port, so tests running in parallel do not share routes or -/// state. Disposing the server stops the listener and completes any request still in flight. +/// The responses are written straight onto a TCP socket rather than through +/// , which on Windows resolves an explicit-address prefix through +/// http.sys and needs a URL reservation an unelevated developer does not have. Each instance binds +/// its own ephemeral port, so tests running in parallel share no routes or state. /// internal sealed class LoopbackServer : IDisposable { @@ -21,8 +23,7 @@ internal sealed class LoopbackServer : IDisposable public const string Content = "loopback content"; /// - /// How long the slow route holds a request before responding, long enough that a caller - /// cancelling after a short delay wins the race deterministically. + /// How long the slow route holds a request before responding, far longer than any test waits. /// public static readonly TimeSpan SlowResponseDelay = TimeSpan.FromSeconds(30); @@ -31,15 +32,22 @@ internal sealed class LoopbackServer : IDisposable /// public LoopbackServer() { - // HttpListener takes a prefix rather than a socket, so a free port is probed and released first. - BaseAddress = new Uri( - FormattableString.Invariant($"http://127.0.0.1:{GetEphemeralPort()}/") - ); - _listener.Prefixes.Add(BaseAddress.AbsoluteUri); + // Binding port 0 and reading the port back leaves no window for another listener to claim it. + _listener = new TcpListener(IPAddress.Loopback, 0); _listener.Start(); - _listenTask = Task.Run(ListenAsync); + + int port = ((IPEndPoint)_listener.LocalEndpoint).Port; + BaseAddress = new Uri(FormattableString.Invariant($"http://127.0.0.1:{port}/")); + + _acceptTask = Task.Run(AcceptAsync); } + /// + /// Gets the byte length of on the wire, which is what a Content-Length + /// header and report. + /// + public static int ContentLength => Encoding.UTF8.GetByteCount(Content); + /// /// Gets the root address the server is listening on. /// @@ -60,125 +68,173 @@ public LoopbackServer() /// public Uri SlowUri => new(BaseAddress, SlowPath); + /// + /// Gets a task that completes once the slow route has received a request, so a test cancels a + /// request the server is known to be holding rather than racing it. + /// + public Task SlowRequestStarted => _slowRequestStarted.Task; + /// /// Stops the listener and waits for the accept loop to finish. /// public void Dispose() { + if (_disposed) + { + return; + } + + _disposed = true; _cancellation.Cancel(); - _listener.Close(); + _listener.Stop(); + _listener.Dispose(); - // The accept loop swallows its own failures, so a stopped listener is not a teardown failure. - _listenTask.GetAwaiter().GetResult(); + // Teardown never fails a test whose assertions already passed. + // The accept loop is waited for without rethrowing its result, and the wait is bounded. + _ = Task.WhenAny(_acceptTask, Task.Delay(s_disposeTimeout)).GetAwaiter().GetResult(); + _ = _acceptTask.Exception; _cancellation.Dispose(); } - private static int GetEphemeralPort() - { - using TcpListener probe = new(IPAddress.Loopback, 0); - probe.Start(); - return ((IPEndPoint)probe.LocalEndpoint).Port; - } - - private async Task ListenAsync() + private async Task AcceptAsync() { while (!_cancellation.IsCancellationRequested) { - HttpListenerContext context; + TcpClient client; try { - context = await _listener.GetContextAsync().ConfigureAwait(false); + client = await _listener + .AcceptTcpClientAsync(_cancellation.Token) + .ConfigureAwait(false); } - catch (HttpListenerException) + catch (OperationCanceledException) { - // The listener was closed by Dispose while waiting for a request. return; } catch (ObjectDisposedException) { return; } + catch (SocketException) + { + return; + } - await RespondAsync(context).ConfigureAwait(false); + using (client) + { + await RespondAsync(client).ConfigureAwait(false); + } } } - private async Task RespondAsync(HttpListenerContext context) + [System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1031:Do not catch general exception types", + Justification = "A test server's accept loop must not fault on one connection's failure." + )] + private async Task RespondAsync(TcpClient client) { - HttpListenerResponse response = context.Response; try { - switch (context.Request.Url?.AbsolutePath) + NetworkStream stream = client.GetStream(); + string target = await ReadRequestTargetAsync(stream).ConfigureAwait(false); + + switch (target) { case "/" + OkPath: - await WriteContentAsync(response).ConfigureAwait(false); + await WriteResponseAsync(stream, 200, "OK", Content).ConfigureAwait(false); break; case "/" + SlowPath: + _ = _slowRequestStarted.TrySetResult(); await Task.Delay(SlowResponseDelay, _cancellation.Token).ConfigureAwait(false); - await WriteContentAsync(response).ConfigureAwait(false); + await WriteResponseAsync(stream, 200, "OK", Content).ConfigureAwait(false); break; default: - response.StatusCode = (int)HttpStatusCode.NotFound; + await WriteResponseAsync(stream, 404, "Not Found", string.Empty) + .ConfigureAwait(false); break; } } - catch (OperationCanceledException) - { - // Dispose ran, or the client went away, while the slow route was waiting. - } - catch (HttpListenerException) - { - // The client disconnected before the response was written, as the cancellation test does. - } - catch (IOException) + catch (Exception) { - // Same disconnect, surfaced from the response stream rather than the listener. - } - finally - { - Close(response); + // A mid-response disconnect and a Dispose cancelling the slow route both land here. } } - private static async Task WriteContentAsync(HttpListenerResponse response) + private async Task ReadRequestTargetAsync(NetworkStream stream) { - byte[] body = Encoding.UTF8.GetBytes(Content); + byte[] buffer = new byte[ReadBufferBytes]; + StringBuilder head = new(); + + while (head.Length < MaxRequestHeadBytes) + { + int read = await stream.ReadAsync(buffer, _cancellation.Token).ConfigureAwait(false); + if (read == 0) + { + break; + } + + // The request line and the headers are ASCII, and only the request line is read. + _ = head.Append(Encoding.ASCII.GetString(buffer, 0, read)); + if (head.ToString().Contains("\r\n\r\n", StringComparison.Ordinal)) + { + break; + } + } - // GetContentInfo reports Content-Length as the size, so it is set rather than left to chunking. - response.StatusCode = (int)HttpStatusCode.OK; - response.ContentType = "text/plain; charset=utf-8"; - response.ContentLength64 = body.Length; - response.Headers.Set("Last-Modified", s_lastModified.ToString("R", null)); + string text = head.ToString(); + int lineEnd = text.IndexOf("\r\n", StringComparison.Ordinal); + string requestLine = lineEnd < 0 ? text : text[..lineEnd]; + string[] parts = requestLine.Split(' '); - await response.OutputStream.WriteAsync(body).ConfigureAwait(false); + return parts.Length >= 2 ? parts[1] : string.Empty; } - private static void Close(HttpListenerResponse response) + private async Task WriteResponseAsync( + NetworkStream stream, + int status, + string reason, + string body + ) { - try - { - response.Close(); - } - catch (HttpListenerException) - { - // Closing a response whose client already disconnected is not a failure. - } - catch (ObjectDisposedException) - { - // The listener was disposed first. - } + byte[] payload = Encoding.UTF8.GetBytes(body); + + // Content-Length is what GetContentInfo reports as the size, so it is always written. + string head = string.Join( + "\r\n", + FormattableString.Invariant($"HTTP/1.1 {status} {reason}"), + "Content-Type: text/plain; charset=utf-8", + FormattableString.Invariant($"Content-Length: {payload.Length}"), + "Last-Modified: " + LastModified, + "Connection: close", + string.Empty, + string.Empty + ); + + await stream + .WriteAsync(Encoding.ASCII.GetBytes(head), _cancellation.Token) + .ConfigureAwait(false); + await stream.WriteAsync(payload, _cancellation.Token).ConfigureAwait(false); + await stream.FlushAsync(_cancellation.Token).ConfigureAwait(false); } private const string OkPath = "ok"; private const string MissingPath = "missing"; private const string SlowPath = "slow"; + private const string LastModified = "Wed, 01 Jan 2020 00:00:00 GMT"; + private const int ReadBufferBytes = 2048; + private const int MaxRequestHeadBytes = 16384; - private static readonly DateTimeOffset s_lastModified = new(2020, 1, 1, 0, 0, 0, TimeSpan.Zero); + private static readonly TimeSpan s_disposeTimeout = TimeSpan.FromSeconds(10); - private readonly HttpListener _listener = new(); + private readonly TcpListener _listener; private readonly CancellationTokenSource _cancellation = new(); - private readonly Task _listenTask; + private readonly TaskCompletionSource _slowRequestStarted = new( + TaskCreationOptions.RunContinuationsAsynchronously + ); + private readonly Task _acceptTask; + private bool _disposed; } diff --git a/UtilitiesTests/StringHistoryTests.cs b/UtilitiesTests/StringHistoryTests.cs index cb95ba0..23a86ea 100644 --- a/UtilitiesTests/StringHistoryTests.cs +++ b/UtilitiesTests/StringHistoryTests.cs @@ -367,4 +367,61 @@ public void Limits_ClearedToZero_ShouldRetainEveryLaterLine() _ = history.StringList[1].Should().Be("Line 9"); _ = history.StringList[2].Should().Be("Line 10"); } + + [Fact] + public void MaxFirstLines_RaisedAfterDiscard_ShouldNotPromoteTailLinesIntoTheHead() + { + StringHistory history = new(maxFirstLines: 2, maxLastLines: 2); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + // The head is closed once a line has been dropped, so raising the limit changes nothing. + history.MaxFirstLines = 5; + for (int i = 10; i < 20; i++) + { + history.AppendLine($"Line {i}"); + } + + _ = history.StringList.Count.Should().Be(4); + _ = history.StringList[0].Should().Be("Line 0"); + _ = history.StringList[1].Should().Be("Line 1"); + _ = history.StringList[2].Should().Be("Line 18"); + _ = history.StringList[3].Should().Be("Line 19"); + } + + [Fact] + public void MaxFirstLines_RaisedBeforeAnyDiscard_ShouldStillFillTheHead() + { + StringHistory history = new(maxFirstLines: 2, maxLastLines: 3); + history.AppendLine("Line 0"); + history.AppendLine("Line 1"); + + // Nothing has been dropped yet, so the head is still open and the larger limit fills. + history.MaxFirstLines = 4; + history.AppendLine("Line 2"); + history.AppendLine("Line 3"); + + _ = history.StringList.Count.Should().Be(4); + _ = history.MaxFirstLines.Should().Be(4); + _ = history.StringList[3].Should().Be("Line 3"); + } + + [Fact] + public void MaxFirstLines_RaisedAfterHeadOnlyDiscard_ShouldNotResumeTheHead() + { + StringHistory history = new(maxFirstLines: 3, maxLastLines: 0); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + // Lines 3 through 9 were dropped, so a larger limit must not start collecting again. + history.MaxFirstLines = 5; + history.AppendLine("Line 10"); + + _ = history.StringList.Count.Should().Be(3); + _ = history.StringList[2].Should().Be("Line 2"); + } } diff --git a/UtilitiesTests/TestHttpDefaults.cs b/UtilitiesTests/TestHttpDefaults.cs new file mode 100644 index 0000000..9b41e91 --- /dev/null +++ b/UtilitiesTests/TestHttpDefaults.cs @@ -0,0 +1,24 @@ +using System.Net; +using System.Runtime.CompilerServices; + +namespace ptr727.Utilities.Tests; + +/// +/// Process-wide HTTP defaults for the test assembly. +/// +internal static class TestHttpDefaults +{ + /// + /// Disables the ambient proxy for every client the tests build. + /// + /// + /// leaves SocketsHttpHandler.UseProxy at its default, so + /// a client resolves , which on Unix is read from + /// http_proxy and its siblings and bypasses loopback only where no_proxy says to. + /// A developer machine or runner configured that way would send every loopback request in + /// 's tests to a proxy, which is the environment dependence these + /// tests exist to remove. A with no address bypasses everything. + /// + [ModuleInitializer] + internal static void DisableAmbientProxy() => HttpClient.DefaultProxy = new WebProxy(); +} From 1932b4e46dc2b724600a37b25307d7c539387795 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 31 Aug 2026 22:56:29 -0700 Subject: [PATCH 03/11] Close the Defects the Second Local Review Found A second adversarial pass over the previous commit found that its own fix had introduced a regression, and a fuzz over the corrected model then found a third defect neither pass had reached. StringHistory. The previous commit tracked one flag for two different questions, so entering the unrestricted mode reset the head counter while leaving the discarded flag set, and the next limit assignment read a partition that no longer described the stored lines. A history limited to 2 and 2, cleared to unrestricted, then given two more lines and limited again, discarded the two most recent lines and kept nothing. A fuzz over the corrected model then found the deeper case: a history that had only ever kept a tail could have a head limit raised, which relabeled tail lines as first lines and left a window that was neither a prefix nor a suffix of the stream. Both are the same root cause, a head that could grow into lines that are not the stream's first, so the head is now closed absolutely: once a line has been discarded the head is trimmed but never refilled, by an append or by a re-partition. The unrestricted mode extends the tail rather than the head once anything has been discarded, so what is retained stays a prefix followed by a suffix. Four tests cover the two cases above, the tail-only history, and a head that cannot be rebuilt. A 300000-history fuzz over random append and limit sequences now holds that prefix-plus-suffix contract across four and a half million states, where the previous model failed it within five iterations. LoopbackServer. Connections are served concurrently rather than one at a time, so the thirty-second slow route cannot block a later request, and Dispose waits for the in-flight connections along with the accept loop under the same bounded, never-rethrowing wait. The server also counts the requests it routes. Tests. The cancellation test waited on a signal that nothing could ever fault or cancel, so a server-side failure before the slow route was reached hung the test rather than failing it, with no deadline anywhere to end the run; the wait is now bounded. Both not-found tests asserted only that the call returned false, which a refused connection produces just as well, so each now asserts the server routed the request. A new test proves two concurrent requests to one server both complete. --- Utilities/StringHistory.cs | 34 ++++++++------- UtilitiesTests/DownloadAsyncTests.cs | 33 ++++++++++++++- UtilitiesTests/DownloadTests.cs | 3 ++ UtilitiesTests/LoopbackServer.cs | 27 +++++++++--- UtilitiesTests/StringHistoryTests.cs | 62 ++++++++++++++++++++++++++++ 5 files changed, 138 insertions(+), 21 deletions(-) diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index f060e8d..8c2ea41 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -48,17 +48,27 @@ public void AppendLine(string value) ArgumentNullException.ThrowIfNull(value); // No restrictions + // The line extends the tail once anything has been discarded, and the head otherwise. + // The counters then keep describing the stored list for a later limit change to read. if (MaxFirstLines == 0 && MaxLastLines == 0) { _stringList.Add(value); - _firstLines++; + if (_discarded) + { + _lastLines++; + } + else + { + _firstLines++; + } + return; } // Restrict first lines // The head only grows while nothing has been discarded. - // A line appended once a tail exists is stored behind it and is not a first line. - if (!_discarded && _firstLines < MaxFirstLines) + // A line appended after a discard is not a first line, whatever the limit now allows. + if (!_discarded && _lastLines == 0 && _firstLines < MaxFirstLines) { _stringList.Add(value); _firstLines++; @@ -152,33 +162,29 @@ public int MaxLastLines /// private void Repartition() { - // Both limits at zero is the unrestricted mode, where every line is retained. - // Nothing is discarded there, so the whole list is head and a later limit slices it freely. + // Both limits at zero is the unrestricted mode, which retains every line. + // Nothing is discarded here, so the counters already describe the stored list. if (MaxFirstLines == 0 && MaxLastLines == 0) { - _firstLines = _stringList.Count; - _lastLines = 0; return; } int storedCount = _stringList.Count; - int firstLines; + + // The head is only ever the stream's own first lines. + // Once a line has been discarded the head is trimmed but never refilled. + // Before any discard the stored lines are the whole stream, so both sides cut from them. + int firstLines = Math.Min(MaxFirstLines, _discarded ? _firstLines : storedCount); int lastLines; if (_discarded) { - // A gap already separates the head from the tail, so the head can only shrink. - // Promoting a tail line into it would pin a line that is not among the first. - firstLines = Math.Min(MaxFirstLines, _firstLines); lastLines = Math.Min(MaxLastLines, _lastLines); _stringList.RemoveRange(firstLines, _firstLines - firstLines); _stringList.RemoveRange(firstLines, _lastLines - lastLines); } else { - // Nothing has been discarded, so the stored lines are the whole stream so far. - // The new head and the new tail are both cut from them. - firstLines = Math.Min(MaxFirstLines, storedCount); lastLines = Math.Min(MaxLastLines, storedCount - firstLines); _stringList.RemoveRange(firstLines, storedCount - firstLines - lastLines); } diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index e1576f0..101acea 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -70,6 +70,31 @@ public async Task GetContentInfoAsync_WithNotFoundUri_ShouldReturnFalse() ); _ = success.Should().BeFalse(); + + // A refused connection returns false too, so the failure is the route's only if it ran. + _ = server.RequestCount.Should().Be(1); + } + + [Fact] + public async Task DownloadAsync_ConcurrentRequests_ShouldBothComplete() + { + using LoopbackServer server = new(); + + // One request must never queue behind another on the server's accept loop. + Task<(bool Success, string Value)> first = Download.DownloadStringAsync( + server.OkUri, + TestContext.Current.CancellationToken + ); + Task<(bool Success, string Value)> second = Download.DownloadStringAsync( + server.OkUri, + TestContext.Current.CancellationToken + ); + + (bool Success, string Value)[] results = await Task.WhenAll(first, second) + .WaitAsync(s_signalTimeout, TestContext.Current.CancellationToken); + + _ = results.Should().AllSatisfy(result => result.Success.Should().BeTrue()); + _ = server.RequestCount.Should().Be(2); } [System.Diagnostics.CodeAnalysis.SuppressMessage( @@ -90,7 +115,10 @@ public async Task DownloadAsync_WithCancellation_ShouldRespectCancellation() // Cancelling only once the server holds the request proves the route was reached. // Any other failure would produce the same false result and prove nothing. - await server.SlowRequestStarted.WaitAsync(TestContext.Current.CancellationToken); + await server.SlowRequestStarted.WaitAsync( + s_signalTimeout, + TestContext.Current.CancellationToken + ); long startedAt = Stopwatch.GetTimestamp(); await cts.CancelAsync(); @@ -114,6 +142,9 @@ public async Task DownloadAsync_WithNullUri_ShouldThrowArgumentNullException() .ThrowAsync(); } + // A signal that never arrives is a defect to report, not a test left hanging in CI. + private static readonly TimeSpan s_signalTimeout = TimeSpan.FromSeconds(30); + [Fact] public void CreateUri_WithCredentials_ShouldIncludeCredentials() { diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index 45cf137..9784214 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -20,5 +20,8 @@ public void GetContentInfo_WithNotFoundUri_ShouldReturnFalse() .GetContentInfo(server.MissingUri, out long _, out DateTime _) .Should() .BeFalse(); + + // A refused connection returns false too, so the failure is the route's only if it ran. + _ = server.RequestCount.Should().Be(1); } } diff --git a/UtilitiesTests/LoopbackServer.cs b/UtilitiesTests/LoopbackServer.cs index 3ee9579..e5d6592 100644 --- a/UtilitiesTests/LoopbackServer.cs +++ b/UtilitiesTests/LoopbackServer.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Net; using System.Net.Sockets; using System.Text; @@ -74,6 +75,12 @@ public LoopbackServer() /// public Task SlowRequestStarted => _slowRequestStarted.Task; + /// + /// Gets the number of requests the server has routed, so a test asserting a failure can prove + /// the request reached the route rather than failing before it. + /// + public int RequestCount => Volatile.Read(ref _requestCount); + /// /// Stops the listener and waits for the accept loop to finish. /// @@ -90,9 +97,10 @@ public void Dispose() _listener.Dispose(); // Teardown never fails a test whose assertions already passed. - // The accept loop is waited for without rethrowing its result, and the wait is bounded. - _ = Task.WhenAny(_acceptTask, Task.Delay(s_disposeTimeout)).GetAwaiter().GetResult(); - _ = _acceptTask.Exception; + // The loops are waited for without rethrowing their results, and the wait is bounded. + Task served = Task.WhenAll([_acceptTask, .. _connections.Keys]); + _ = Task.WhenAny(served, Task.Delay(s_disposeTimeout)).GetAwaiter().GetResult(); + _ = served.Exception; _cancellation.Dispose(); } @@ -121,10 +129,14 @@ private async Task AcceptAsync() return; } - using (client) + Task connection = Task.Run(async () => { - await RespondAsync(client).ConfigureAwait(false); - } + using (client) + { + await RespondAsync(client).ConfigureAwait(false); + } + }); + _ = _connections.TryAdd(connection, 0); } } @@ -139,6 +151,7 @@ private async Task RespondAsync(TcpClient client) { NetworkStream stream = client.GetStream(); string target = await ReadRequestTargetAsync(stream).ConfigureAwait(false); + _ = Interlocked.Increment(ref _requestCount); switch (target) { @@ -236,5 +249,7 @@ await stream TaskCreationOptions.RunContinuationsAsynchronously ); private readonly Task _acceptTask; + private readonly ConcurrentDictionary _connections = new(); + private int _requestCount; private bool _disposed; } diff --git a/UtilitiesTests/StringHistoryTests.cs b/UtilitiesTests/StringHistoryTests.cs index 23a86ea..5950cd1 100644 --- a/UtilitiesTests/StringHistoryTests.cs +++ b/UtilitiesTests/StringHistoryTests.cs @@ -424,4 +424,66 @@ public void MaxFirstLines_RaisedAfterHeadOnlyDiscard_ShouldNotResumeTheHead() _ = history.StringList.Count.Should().Be(3); _ = history.StringList[2].Should().Be("Line 2"); } + + [Fact] + public void MaxFirstLines_RaisedOnATailOnlyHistory_ShouldNotAdoptTailLinesAsFirstLines() + { + StringHistory history = new(maxFirstLines: 0, maxLastLines: 5); + for (int i = 0; i < 6; i++) + { + history.AppendLine($"Line {i}"); + } + + // Line 0 is already gone, so no first line survives for a head to be built from. + // The retained lines stay the tail, rolling as before. + history.MaxFirstLines = 5; + history.AppendLine("Line 6"); + + _ = history.StringList.Count.Should().Be(5); + _ = history.StringList[0].Should().Be("Line 2"); + _ = history.StringList[4].Should().Be("Line 6"); + } + + [Fact] + public void UnrestrictedAfterDiscard_ThenLimited_ShouldKeepTheMostRecentLines() + { + StringHistory history = new(maxFirstLines: 2, maxLastLines: 2); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + history.MaxFirstLines = 0; + history.MaxLastLines = 0; + history.AppendLine("Line 10"); + history.AppendLine("Line 11"); + + // The lines added while unrestricted extend the tail. + // Limiting the tail again keeps them rather than the older ones the earlier limits kept. + history.MaxLastLines = 2; + + _ = history.StringList.Count.Should().Be(2); + _ = history.StringList[0].Should().Be("Line 10"); + _ = history.StringList[1].Should().Be("Line 11"); + } + + [Fact] + public void ClearingMaxFirstLines_ThenLimitingItAgain_ShouldNotRebuildTheHead() + { + StringHistory history = new(maxFirstLines: 2, maxLastLines: 2); + for (int i = 0; i < 10; i++) + { + history.AppendLine($"Line {i}"); + } + + // Clearing MaxFirstLines retains no first lines, so it discards the head it had. + history.MaxFirstLines = 0; + history.MaxLastLines = 0; + history.AppendLine("Line 10"); + + // No first line survives for a head, and MaxLastLines is 0 by now, so neither side keeps. + history.MaxFirstLines = 2; + + _ = history.StringList.Should().BeEmpty(); + } } From f7e0006eaf7a9b0e892f1ead8d6e4dab575bc51e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 06:28:37 -0700 Subject: [PATCH 04/11] Replace the Destination File on a Download Instead of Overwriting It DownloadFile and DownloadFileAsync opened the destination with File.OpenWrite, which is FileMode.OpenOrCreate and does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and returned true. A 16-byte body written over a 200-byte file produced a 200-byte file. Both now create the file instead. Both local review passes raised this against the download tests rather than the library, since the rewritten test exercises the path for real but seeds it with Path.GetTempFileName's empty file, so its length assertion passed either way. Two tests now seed a file four times the body's length first, and both fail against the previous behavior and pass against this one. The maintainer asked for it here rather than as a separate issue, so ARCHITECTURE.md records the contract and README.md and HISTORY.md carry the fix. --- ARCHITECTURE.md | 2 +- HISTORY.md | 1 + README.md | 1 + Utilities/Download.cs | 7 +++-- UtilitiesTests/DownloadAsyncTests.cs | 38 ++++++++++++++++++++++++++++ UtilitiesTests/DownloadTests.cs | 24 ++++++++++++++++++ 6 files changed, 70 insertions(+), 3 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 60bb31d..a3453b3 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -18,7 +18,7 @@ These are behavioral contracts rather than formatting rules, which is why they l - **I/O methods return `bool`** for success or failure, and hand back any additional result through an `out` parameter. They do not signal ordinary I/O failure by throwing. - **Async methods carry the `Async` suffix** and take an optional `CancellationToken cancellationToken = default`, passed through to the underlying call rather than ignored. An async overload returning several values returns a tuple, since `out` parameters are unavailable there. -- **`Download`** reuses a thread-safe `Lazy` and reads with `HttpCompletionOption.ResponseHeadersRead`, so a large response streams rather than buffering whole. +- **`Download`** reuses a thread-safe `Lazy` and reads with `HttpCompletionOption.ResponseHeadersRead`, so a large response streams rather than buffering whole. A download to a file replaces that file, so a response shorter than the file already there leaves nothing of it behind. - **`FileEx`** wraps its I/O in retry logic configured through `Options`, and honors cancellation from both `Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate, takes a configurable compression level, and passes `leaveOpen` so the caller keeps ownership of the stream it supplied. - **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Both limits at zero is the one unrestricted mode, and zero on a single side retains no lines on that side. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. diff --git a/HISTORY.md b/HISTORY.md index a8f82b7..f693b74 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,6 +5,7 @@ Some useful and not so useful C# .NET utility classes. ## Release History - v4.0: + - Fixed `Download.DownloadFile` and `DownloadFileAsync` truncating the destination: both opened it with `File.OpenWrite`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later append, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`; it exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. - Added `AssemblyInfo`, an AOT safe assembly and application identity helper whose `For()` substitutes for `Assembly.GetExecutingAssembly()` (unreliable under Native AOT), and which supplies the consuming application name, version, and a default User-Agent. diff --git a/README.md b/README.md index ebdb48b..4f47ac5 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,7 @@ Some useful and not so useful C# .NET utility classes. **Summary**: - Added `HttpClientFactory`, a reusable resilient HTTP client factory (Polly retry and circuit breaker) with an AOT safe `AssemblyInfo` identity helper and a tunable `HttpClientOptions`. +- Fixed `Download.DownloadFile` and `DownloadFileAsync` leaving trailing bytes when downloading over a longer existing file. See [Release History](./HISTORY.md) for complete release notes and older versions. diff --git a/Utilities/Download.cs b/Utilities/Download.cs index de4694f..5db248a 100644 --- a/Utilities/Download.cs +++ b/Utilities/Download.cs @@ -102,7 +102,9 @@ public static bool DownloadFile(Uri uri, string fileName) try { using Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult(); - using FileStream fileStream = File.OpenWrite(fileName); + + // Created rather than opened, so a shorter download replaces an existing file whole. + using FileStream fileStream = File.Create(fileName); httpStream.CopyTo(fileStream); } catch (Exception e) when (Log.LogAndHandle(e)) @@ -137,7 +139,8 @@ public static async Task DownloadFileAsync( .ConfigureAwait(false); await using (httpStream.ConfigureAwait(false)) { - FileStream fileStream = File.OpenWrite(fileName); + // Created rather than opened, so a shorter download replaces an existing file whole. + FileStream fileStream = File.Create(fileName); await using (fileStream.ConfigureAwait(false)) { await httpStream diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index 101acea..2e5d4b3 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -59,6 +59,44 @@ public async Task DownloadFileAsync_WithValidUri_ShouldCreateFile() } } + [Fact] + public async Task DownloadFileAsync_OverALongerExistingFile_ShouldReplaceIt() + { + using LoopbackServer server = new(); + string tempFile = Path.GetTempFileName(); + + try + { + await File.WriteAllTextAsync( + tempFile, + new string('x', LoopbackServer.ContentLength * 4), + TestContext.Current.CancellationToken + ); + + bool result = await Download.DownloadFileAsync( + server.OkUri, + tempFile, + TestContext.Current.CancellationToken + ); + + _ = result.Should().BeTrue(); + + // Opening rather than creating the file would leave the seeded bytes after the body. + string written = await File.ReadAllTextAsync( + tempFile, + TestContext.Current.CancellationToken + ); + _ = written.Should().Be(LoopbackServer.Content); + } + finally + { + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + } + } + [Fact] public async Task GetContentInfoAsync_WithNotFoundUri_ShouldReturnFalse() { diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index 9784214..b782c36 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -11,6 +11,30 @@ public void GetUriInformation() _ = size.Should().Be(LoopbackServer.ContentLength); } + [Fact] + public void DownloadFile_OverALongerExistingFile_ShouldReplaceIt() + { + using LoopbackServer server = new(); + string tempFile = Path.GetTempFileName(); + + try + { + File.WriteAllText(tempFile, new string('x', LoopbackServer.ContentLength * 4)); + + _ = Download.DownloadFile(server.OkUri, tempFile).Should().BeTrue(); + + // Opening rather than creating the file would leave the seeded bytes after the body. + _ = File.ReadAllText(tempFile).Should().Be(LoopbackServer.Content); + } + finally + { + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + } + } + [Fact] public void GetContentInfo_WithNotFoundUri_ShouldReturnFalse() { From 6935b652d571bb646c987e959edcfb9ef431a066 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 06:44:27 -0700 Subject: [PATCH 05/11] Replace a Downloaded File Only Once the Whole Body Is on Disk Answers the first review round on the pull request and a third local pass. Download. The previous commit fixed the destination not being truncated by creating the file instead of opening it, which traded one defect for a worse one: GetStreamAsync returns once the headers arrive, so the destination was emptied before a single body byte existed and a download that then failed left a 0-byte file where the old code left the original content. File.Create also asks for FileAccess.ReadWrite, so a destination the process may write but not read stopped working at all. Both methods now write beside the destination and move the result into place, so the destination is replaced whole on success and left untouched on failure, and the temporary file is removed when a download fails. Two tests cover the failure case, alongside the two covering the overwrite. ARCHITECTURE.md said Download reads with HttpCompletionOption.ResponseHeadersRead so a large response streams rather than buffering whole. That is true only of GetContentInfo, which discards the body; DownloadString calls GetStringAsync and buffers the whole response. The entry now says which method does which. StringHistory. The constructor assigns the backing fields rather than the properties, since both limits apply as one pair and the setters would apply them one at a time and re-partition an empty history twice. The XML documentation and the comments drop the "both limits at zero is" construction that four findings across three files objected to. HISTORY.md now also records the AppendLine behavior that shipped with the setters, which it described only as a limit set after appending being honored. LoopbackServer. The 404 route has its own case rather than reaching the default arm, so the route contract is stated rather than incidental. A peer that connects and sends nothing is no longer counted as a routed request, which the exact-count assertions depend on. Dispose waits for the accept loop before reading the connection set, so the set is final rather than a snapshot that could miss a connection started but not yet registered. The 300000-history fuzz continues to hold the prefix-plus-suffix contract, the count bound, no duplicates, and that an assignment never adds a line, across four and a half million states. --- ARCHITECTURE.md | 4 +- HISTORY.md | 4 +- README.md | 2 +- Utilities/Download.cs | 61 +++++++++++++++++++++++++--- Utilities/StringHistory.cs | 20 +++++---- UtilitiesTests/DownloadAsyncTests.cs | 35 ++++++++++++++++ UtilitiesTests/DownloadTests.cs | 25 ++++++++++++ UtilitiesTests/LoopbackServer.cs | 19 ++++++++- 8 files changed, 148 insertions(+), 22 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index a3453b3..b1e8b56 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -18,9 +18,9 @@ These are behavioral contracts rather than formatting rules, which is why they l - **I/O methods return `bool`** for success or failure, and hand back any additional result through an `out` parameter. They do not signal ordinary I/O failure by throwing. - **Async methods carry the `Async` suffix** and take an optional `CancellationToken cancellationToken = default`, passed through to the underlying call rather than ignored. An async overload returning several values returns a tuple, since `out` parameters are unavailable there. -- **`Download`** reuses a thread-safe `Lazy` and reads with `HttpCompletionOption.ResponseHeadersRead`, so a large response streams rather than buffering whole. A download to a file replaces that file, so a response shorter than the file already there leaves nothing of it behind. +- **`Download`** reuses a thread-safe `Lazy`. `GetContentInfo` reads with `HttpCompletionOption.ResponseHeadersRead`, so asking for a size never fetches the body, while `DownloadString` buffers the whole response and a large body belongs in `DownloadFile` instead. A download to a file writes beside the destination and moves the result into place, so the destination is replaced whole on success and left untouched on failure. - **`FileEx`** wraps its I/O in retry logic configured through `Options`, and honors cancellation from both `Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate, takes a configurable compression level, and passes `leaveOpen` so the caller keeps ownership of the stream it supplied. -- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Both limits at zero is the one unrestricted mode, and zero on a single side retains no lines on that side. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. +- **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Zero on a single side retains no lines on that side, and zero on both is the one unrestricted mode. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. - **`Extensions`** uses the C# `extension` block form inside a static class for its logger and string helpers. - **Logging is a seam, never a dependency.** The library depends on `Microsoft.Extensions.Logging.Abstractions` and takes an `ILoggerFactory` through `LogOptions`. It references no logging framework or sink, so a consumer chooses its own. `Serilog` appears only in `Sandbox` and the tests, where an application legitimately picks one. diff --git a/HISTORY.md b/HISTORY.md index f693b74..99ad38e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,8 +5,8 @@ Some useful and not so useful C# .NET utility classes. ## Release History - v4.0: - - Fixed `Download.DownloadFile` and `DownloadFileAsync` truncating the destination: both opened it with `File.OpenWrite`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. - - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later append, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. + - Fixed `Download.DownloadFile` and `DownloadFileAsync` corrupting the destination file: both opened it with `File.OpenWrite`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now write beside the destination and move the result into place, so the destination is replaced whole on success and left untouched when a download fails partway. + - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later append, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. Appending changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`; it exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. - Added `AssemblyInfo`, an AOT safe assembly and application identity helper whose `For()` substitutes for `Assembly.GetExecutingAssembly()` (unreliable under Native AOT), and which supplies the consuming application name, version, and a default User-Agent. - Reworked `Download` to build its `HttpClient` through `HttpClientFactory`, so downloads now flow through the shared retry and circuit-breaker pipeline; the `TimeoutSeconds` property and all method signatures are unchanged. diff --git a/README.md b/README.md index 4f47ac5..f181e2d 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ Some useful and not so useful C# .NET utility classes. **Summary**: - Added `HttpClientFactory`, a reusable resilient HTTP client factory (Polly retry and circuit breaker) with an AOT safe `AssemblyInfo` identity helper and a tunable `HttpClientOptions`. -- Fixed `Download.DownloadFile` and `DownloadFileAsync` leaving trailing bytes when downloading over a longer existing file. +- Fixed `Download.DownloadFile` and `DownloadFileAsync` corrupting the destination when downloading over a longer existing file. The destination is now replaced whole on success and left untouched on failure. See [Release History](./HISTORY.md) for complete release notes and older versions. diff --git a/Utilities/Download.cs b/Utilities/Download.cs index 5db248a..e28242d 100644 --- a/Utilities/Download.cs +++ b/Utilities/Download.cs @@ -99,16 +99,29 @@ public static bool DownloadFile(Uri uri, string fileName) ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(fileName); + string tempFileName = CreateTemporaryPath(fileName); try { - using Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult(); + using (Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult()) + using ( + FileStream fileStream = new( + tempFileName, + FileMode.Create, + FileAccess.Write, + FileShare.None + ) + ) + { + httpStream.CopyTo(fileStream); + } - // Created rather than opened, so a shorter download replaces an existing file whole. - using FileStream fileStream = File.Create(fileName); - httpStream.CopyTo(fileStream); + // The destination is replaced only once the whole body is on disk. + // A download that fails partway therefore leaves what was already there untouched. + File.Move(tempFileName, fileName, overwrite: true); } catch (Exception e) when (Log.LogAndHandle(e)) { + DeleteTemporary(tempFileName); return false; } @@ -132,6 +145,7 @@ public static async Task DownloadFileAsync( ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(fileName); + string tempFileName = CreateTemporaryPath(fileName); try { Stream httpStream = await GetHttpClient() @@ -139,8 +153,12 @@ public static async Task DownloadFileAsync( .ConfigureAwait(false); await using (httpStream.ConfigureAwait(false)) { - // Created rather than opened, so a shorter download replaces an existing file whole. - FileStream fileStream = File.Create(fileName); + FileStream fileStream = new( + tempFileName, + FileMode.Create, + FileAccess.Write, + FileShare.None + ); await using (fileStream.ConfigureAwait(false)) { await httpStream @@ -148,9 +166,14 @@ await httpStream .ConfigureAwait(false); } } + + // The destination is replaced only once the whole body is on disk. + // A download that fails partway therefore leaves what was already there untouched. + File.Move(tempFileName, fileName, overwrite: true); } catch (Exception e) when (Log.LogAndHandle(e)) { + DeleteTemporary(tempFileName); return false; } @@ -219,6 +242,32 @@ public static bool DownloadString(Uri uri, out string value) )] public static HttpClient GetHttpClient() => s_httpClient.Value; + private static string CreateTemporaryPath(string fileName) + { + // The temporary file sits beside the destination, so the replacing move stays on one volume. + string? directory = Path.GetDirectoryName(fileName); + return Path.Combine( + string.IsNullOrEmpty(directory) ? "." : directory, + Path.GetRandomFileName() + ); + } + + private static void DeleteTemporary(string fileName) + { + try + { + File.Delete(fileName); + } + catch (IOException) + { + // A partial download left behind is not itself a download failure. + } + catch (UnauthorizedAccessException) + { + // Same, where the file or its directory denies the delete. + } + } + /// /// Creates a URI with optional username and password credentials. /// diff --git a/Utilities/StringHistory.cs b/Utilities/StringHistory.cs index 8c2ea41..3ca05c5 100644 --- a/Utilities/StringHistory.cs +++ b/Utilities/StringHistory.cs @@ -5,9 +5,9 @@ namespace ptr727.Utilities; /// /// /// This class is useful for maintaining a bounded history buffer, keeping the first N and last M lines -/// while discarding intermediate content when limits are exceeded. Both limits at zero is the one -/// unrestricted mode, where every appended line is retained; zero on a single side retains no lines -/// on that side. A limit assigned after lines have been appended re-partitions what is already +/// while discarding intermediate content when limits are exceeded. Zero on a single side retains no +/// lines on that side, and zero on both is the one unrestricted mode, where every appended line is +/// retained. A limit assigned after lines have been appended re-partitions what is already /// stored, so the history never holds more than the limits then in force allow. Re-partitioning /// only ever discards: once a line has been dropped the head is closed, and a later, larger /// raises the ceiling without recovering or repopulating it. @@ -30,12 +30,14 @@ public class StringHistory public StringHistory(int maxFirstLines, int maxLastLines) : this() { - // Validated here rather than in the setters so the exception names the caller's parameter. + // Validated here so the exception names the caller's own parameter rather than "value". ArgumentOutOfRangeException.ThrowIfNegative(maxFirstLines); ArgumentOutOfRangeException.ThrowIfNegative(maxLastLines); - MaxFirstLines = maxFirstLines; - MaxLastLines = maxLastLines; + // Assigned to the fields because the two limits apply together. + // The setters would apply them one at a time and re-partition an empty history twice. + _maxFirstLines = maxFirstLines; + _maxLastLines = maxLastLines; } /// @@ -107,7 +109,7 @@ public override string ToString() => /// /// Gets or sets the maximum number of first lines to retain. - /// Set to 0 to retain no first lines; both limits at 0 retains every line. + /// Set to 0 to retain no first lines. Every line is retained only when both limits are 0. /// /// /// Assigning this re-partitions the lines already stored against the limits then in force, @@ -130,7 +132,7 @@ public int MaxFirstLines /// /// Gets or sets the maximum number of last lines to retain. - /// Set to 0 to retain no last lines; both limits at 0 retains every line. + /// Set to 0 to retain no last lines. Every line is retained only when both limits are 0. /// /// /// Assigning this re-partitions the lines already stored against the limits then in force, @@ -162,7 +164,7 @@ public int MaxLastLines /// private void Repartition() { - // Both limits at zero is the unrestricted mode, which retains every line. + // Zero on both limits is the unrestricted mode, which retains every line. // Nothing is discarded here, so the counters already describe the stored list. if (MaxFirstLines == 0 && MaxLastLines == 0) { diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index 2e5d4b3..4d30ba3 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -97,6 +97,41 @@ await File.WriteAllTextAsync( } } + [Fact] + public async Task DownloadFileAsync_WhenTheDownloadFails_ShouldLeaveTheDestination() + { + using LoopbackServer server = new(); + string tempFile = Path.GetTempFileName(); + string existing = new('x', LoopbackServer.ContentLength * 4); + + try + { + await File.WriteAllTextAsync(tempFile, existing, TestContext.Current.CancellationToken); + + bool result = await Download.DownloadFileAsync( + server.MissingUri, + tempFile, + TestContext.Current.CancellationToken + ); + + _ = result.Should().BeFalse(); + + // Truncating the destination at open time would leave an empty file here. + string written = await File.ReadAllTextAsync( + tempFile, + TestContext.Current.CancellationToken + ); + _ = written.Should().Be(existing); + } + finally + { + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + } + } + [Fact] public async Task GetContentInfoAsync_WithNotFoundUri_ShouldReturnFalse() { diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index b782c36..ec4262d 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -35,6 +35,31 @@ public void DownloadFile_OverALongerExistingFile_ShouldReplaceIt() } } + [Fact] + public void DownloadFile_WhenTheDownloadFails_ShouldLeaveTheDestination() + { + using LoopbackServer server = new(); + string tempFile = Path.GetTempFileName(); + string existing = new('x', LoopbackServer.ContentLength * 4); + + try + { + File.WriteAllText(tempFile, existing); + + _ = Download.DownloadFile(server.MissingUri, tempFile).Should().BeFalse(); + + // Truncating the destination at open time would leave an empty file here. + _ = File.ReadAllText(tempFile).Should().Be(existing); + } + finally + { + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + } + } + [Fact] public void GetContentInfo_WithNotFoundUri_ShouldReturnFalse() { diff --git a/UtilitiesTests/LoopbackServer.cs b/UtilitiesTests/LoopbackServer.cs index e5d6592..3e63c25 100644 --- a/UtilitiesTests/LoopbackServer.cs +++ b/UtilitiesTests/LoopbackServer.cs @@ -97,8 +97,13 @@ public void Dispose() _listener.Dispose(); // Teardown never fails a test whose assertions already passed. - // The loops are waited for without rethrowing their results, and the wait is bounded. - Task served = Task.WhenAll([_acceptTask, .. _connections.Keys]); + // The loops are waited for without rethrowing their results, and each wait is bounded. + // The accept loop is the only thing that adds a connection. + // Waiting for it first is what makes the set final rather than a mid-iteration snapshot. + _ = Task.WhenAny(_acceptTask, Task.Delay(s_disposeTimeout)).GetAwaiter().GetResult(); + _ = _acceptTask.Exception; + + Task served = Task.WhenAll(_connections.Keys); _ = Task.WhenAny(served, Task.Delay(s_disposeTimeout)).GetAwaiter().GetResult(); _ = served.Exception; @@ -151,6 +156,14 @@ private async Task RespondAsync(TcpClient client) { NetworkStream stream = client.GetStream(); string target = await ReadRequestTargetAsync(stream).ConfigureAwait(false); + + // A peer that connects and sends nothing is not a request. + // Counting it would make the exact-count assertions depend on whether one happened. + if (target.Length == 0) + { + return; + } + _ = Interlocked.Increment(ref _requestCount); switch (target) @@ -165,6 +178,8 @@ private async Task RespondAsync(TcpClient client) await WriteResponseAsync(stream, 200, "OK", Content).ConfigureAwait(false); break; + // The default arm answers the same way, so this case is what names the route. + case "/" + MissingPath: default: await WriteResponseAsync(stream, 404, "Not Found", string.Empty) .ConfigureAwait(false); From 7aac1025cb7a9499148eb999ffd49228f4f51a10 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 07:00:34 -0700 Subject: [PATCH 06/11] Create the Download Temporary File Without Overwriting a Name in Use Answers the review round on 6935b65, which raised this against both DownloadFile and DownloadFileAsync. The temporary file was opened with FileMode.Create under a name from Path.GetRandomFileName, so a name that happened to exist in the destination directory was overwritten, and the failure path then deleted that unrelated file. The file is now created with FileMode.CreateNew, which fails rather than overwrites, and a collision retries under a fresh name. The path is assigned to the caller's variable only once the file behind it exists, so the failure path deletes a file this call actually created and nothing else. Any other I/O failure, a missing destination directory among them, is left to the caller to report rather than retried. The same finding also asked that the temporary path be named inside the try, since Path.GetDirectoryName can throw on invalid path characters and would otherwise break the bool-returning contract. Naming now happens inside the try, so the point is moot either way, but the premise does not hold on this target framework: on net10.0 Path.GetDirectoryName and Path.Combine return normally for an embedded null character, a pipe, and a 5000-character path alike. That rejection is .NET Framework behavior. Two tests cover a destination whose directory does not exist, one per method, asserting the call reports failure rather than throwing and leaves no file behind. --- Utilities/Download.cs | 66 +++++++++++++++++++--------- UtilitiesTests/DownloadAsyncTests.cs | 22 ++++++++++ UtilitiesTests/DownloadTests.cs | 16 +++++++ 3 files changed, 83 insertions(+), 21 deletions(-) diff --git a/Utilities/Download.cs b/Utilities/Download.cs index e28242d..ab0aad9 100644 --- a/Utilities/Download.cs +++ b/Utilities/Download.cs @@ -5,6 +5,8 @@ namespace ptr727.Utilities; /// public static class Download { + private const int TemporaryFileAttempts = 5; + private static readonly Lazy s_httpClient = new(() => HttpClientFactory.CreateClient( new HttpClientOptions { Timeout = TimeSpan.FromSeconds(TimeoutSeconds) } @@ -99,18 +101,12 @@ public static bool DownloadFile(Uri uri, string fileName) ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(fileName); - string tempFileName = CreateTemporaryPath(fileName); + // Named only once the file behind it exists, so a failure before that deletes nothing. + string tempFileName = string.Empty; try { using (Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult()) - using ( - FileStream fileStream = new( - tempFileName, - FileMode.Create, - FileAccess.Write, - FileShare.None - ) - ) + using (FileStream fileStream = CreateTemporaryFile(fileName, out tempFileName)) { httpStream.CopyTo(fileStream); } @@ -145,7 +141,8 @@ public static async Task DownloadFileAsync( ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(fileName); - string tempFileName = CreateTemporaryPath(fileName); + // Named only once the file behind it exists, so a failure before that deletes nothing. + string tempFileName = string.Empty; try { Stream httpStream = await GetHttpClient() @@ -153,12 +150,7 @@ public static async Task DownloadFileAsync( .ConfigureAwait(false); await using (httpStream.ConfigureAwait(false)) { - FileStream fileStream = new( - tempFileName, - FileMode.Create, - FileAccess.Write, - FileShare.None - ); + FileStream fileStream = CreateTemporaryFile(fileName, out tempFileName); await using (fileStream.ConfigureAwait(false)) { await httpStream @@ -242,18 +234,50 @@ public static bool DownloadString(Uri uri, out string value) )] public static HttpClient GetHttpClient() => s_httpClient.Value; - private static string CreateTemporaryPath(string fileName) + [System.Diagnostics.CodeAnalysis.SuppressMessage( + "Reliability", + "CA2000:Dispose objects before losing scope", + Justification = "Ownership of the stream transfers to the caller, which disposes it." + )] + private static FileStream CreateTemporaryFile(string fileName, out string tempFileName) { // The temporary file sits beside the destination, so the replacing move stays on one volume. string? directory = Path.GetDirectoryName(fileName); - return Path.Combine( - string.IsNullOrEmpty(directory) ? "." : directory, - Path.GetRandomFileName() - ); + string parent = string.IsNullOrEmpty(directory) ? "." : directory; + + for (int attempt = 0; ; attempt++) + { + string candidate = Path.Combine(parent, Path.GetRandomFileName()); + try + { + // CreateNew rather than Create, so a name already taken is never overwritten. + FileStream stream = new( + candidate, + FileMode.CreateNew, + FileAccess.Write, + FileShare.None + ); + + // Assigned only here, so the caller deletes a path this call actually created. + tempFileName = candidate; + return stream; + } + catch (IOException) when (attempt < TemporaryFileAttempts && File.Exists(candidate)) + { + // A random name collided, and another name is the whole remedy. + // Any other IO failure, a missing directory among them, is the caller's to report. + } + } } private static void DeleteTemporary(string fileName) { + // Empty means no temporary file was created, so there is nothing of this call's to remove. + if (string.IsNullOrEmpty(fileName)) + { + return; + } + try { File.Delete(fileName); diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index 4d30ba3..cfe19ee 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -132,6 +132,28 @@ public async Task DownloadFileAsync_WhenTheDownloadFails_ShouldLeaveTheDestinati } } + [Fact] + public async Task DownloadFileAsync_WithAnUnusableDestination_ShouldReturnFalse() + { + using LoopbackServer server = new(); + string missingDirectory = Path.Combine( + Path.GetTempPath(), + Path.GetRandomFileName(), + "file.txt" + ); + + // Naming the temporary file is part of the download. + // A destination that cannot hold one reports failure rather than throwing. + bool result = await Download.DownloadFileAsync( + server.OkUri, + missingDirectory, + TestContext.Current.CancellationToken + ); + + _ = result.Should().BeFalse(); + _ = File.Exists(missingDirectory).Should().BeFalse(); + } + [Fact] public async Task GetContentInfoAsync_WithNotFoundUri_ShouldReturnFalse() { diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index ec4262d..40b25c7 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -60,6 +60,22 @@ public void DownloadFile_WhenTheDownloadFails_ShouldLeaveTheDestination() } } + [Fact] + public void DownloadFile_WithAnUnusableDestination_ShouldReturnFalse() + { + using LoopbackServer server = new(); + string missingDirectory = Path.Combine( + Path.GetTempPath(), + Path.GetRandomFileName(), + "file.txt" + ); + + // Naming the temporary file is part of the download. + // A destination that cannot hold one reports failure rather than throwing. + _ = Download.DownloadFile(server.OkUri, missingDirectory).Should().BeFalse(); + _ = File.Exists(missingDirectory).Should().BeFalse(); + } + [Fact] public void GetContentInfo_WithNotFoundUri_ShouldReturnFalse() { From 2bc77c0675282afb84d4b129dac4eea2b380db97 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 07:15:01 -0700 Subject: [PATCH 07/11] Truncate and Rewrite a Downloaded File in Place The maintainer chose write-in-place over the temporary-file-and-move approach the two previous commits built, after a local review pass established what that approach cost. Moving a temporary file into place replaces the destination's identity rather than its contents, which a probe confirmed on Linux: a destination at mode 0600 came back at 0664, a symlink destination was replaced by a regular file leaving its target stale, a second hard link kept the old content, and a read-only destination was silently overwritten, because rename consults the directory's permission and ignores the file's. Windows refuses that same move, so the two platforms disagreed on whether the download succeeded. A killed process also orphaned an unnamed temporary in the destination's own directory. Both methods now open the destination with FileMode.Create and FileAccess.Write. Create is what fixes the original defect, since OpenWrite does not truncate and left a longer file's trailing bytes after the body. Write rather than the ReadWrite that File.Create requests, so a destination the process may write but not read still works. A probe against this build confirms the four properties the documentation now claims: mode 0600 survives with the file truncated to the body's length, a hard link sees the new content, a symlink is written through rather than replaced, and a read-only destination is refused with the file left intact. The cost, stated in ARCHITECTURE.md rather than left implicit: the destination is opened once the response is accepted, so a failure during the body can leave a partial file. A request that fails before that leaves it untouched, which is what the two renamed tests pin, and they now assert the server served the request so a connection failure cannot pass for the file behaving correctly. CreateTemporaryFile and DeleteTemporary are gone with the approach that needed them. --- ARCHITECTURE.md | 2 +- Utilities/Download.cs | 99 ++++++---------------------- UtilitiesTests/DownloadAsyncTests.cs | 13 ++-- UtilitiesTests/DownloadTests.cs | 13 ++-- 4 files changed, 36 insertions(+), 91 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b1e8b56..cf61938 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -18,7 +18,7 @@ These are behavioral contracts rather than formatting rules, which is why they l - **I/O methods return `bool`** for success or failure, and hand back any additional result through an `out` parameter. They do not signal ordinary I/O failure by throwing. - **Async methods carry the `Async` suffix** and take an optional `CancellationToken cancellationToken = default`, passed through to the underlying call rather than ignored. An async overload returning several values returns a tuple, since `out` parameters are unavailable there. -- **`Download`** reuses a thread-safe `Lazy`. `GetContentInfo` reads with `HttpCompletionOption.ResponseHeadersRead`, so asking for a size never fetches the body, while `DownloadString` buffers the whole response and a large body belongs in `DownloadFile` instead. A download to a file writes beside the destination and moves the result into place, so the destination is replaced whole on success and left untouched on failure. +- **`Download`** reuses a thread-safe `Lazy`. `GetContentInfo` reads with `HttpCompletionOption.ResponseHeadersRead`, so asking for a size never fetches the body, while `DownloadString` buffers the whole response and a large body belongs in `DownloadFile` instead. A download to a file truncates and rewrites the destination in place, so its permissions, ownership, and any links to it survive the download, and a request that fails before the response is accepted leaves it untouched. The destination is opened once the response is accepted, so a failure during the body can leave a partial file. - **`FileEx`** wraps its I/O in retry logic configured through `Options`, and honors cancellation from both `Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate, takes a configurable compression level, and passes `leaveOpen` so the caller keeps ownership of the stream it supplied. - **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Zero on a single side retains no lines on that side, and zero on both is the one unrestricted mode. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. diff --git a/Utilities/Download.cs b/Utilities/Download.cs index ab0aad9..b8ba349 100644 --- a/Utilities/Download.cs +++ b/Utilities/Download.cs @@ -5,8 +5,6 @@ namespace ptr727.Utilities; /// public static class Download { - private const int TemporaryFileAttempts = 5; - private static readonly Lazy s_httpClient = new(() => HttpClientFactory.CreateClient( new HttpClientOptions { Timeout = TimeSpan.FromSeconds(TimeoutSeconds) } @@ -101,23 +99,22 @@ public static bool DownloadFile(Uri uri, string fileName) ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(fileName); - // Named only once the file behind it exists, so a failure before that deletes nothing. - string tempFileName = string.Empty; try { - using (Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult()) - using (FileStream fileStream = CreateTemporaryFile(fileName, out tempFileName)) - { - httpStream.CopyTo(fileStream); - } - - // The destination is replaced only once the whole body is on disk. - // A download that fails partway therefore leaves what was already there untouched. - File.Move(tempFileName, fileName, overwrite: true); + using Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult(); + + // Create truncates where OpenWrite left a longer file's tail behind the body. + // Rewriting in place keeps the destination's own permissions and links. + using FileStream fileStream = new( + fileName, + FileMode.Create, + FileAccess.Write, + FileShare.None + ); + httpStream.CopyTo(fileStream); } catch (Exception e) when (Log.LogAndHandle(e)) { - DeleteTemporary(tempFileName); return false; } @@ -141,8 +138,6 @@ public static async Task DownloadFileAsync( ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(fileName); - // Named only once the file behind it exists, so a failure before that deletes nothing. - string tempFileName = string.Empty; try { Stream httpStream = await GetHttpClient() @@ -150,7 +145,14 @@ public static async Task DownloadFileAsync( .ConfigureAwait(false); await using (httpStream.ConfigureAwait(false)) { - FileStream fileStream = CreateTemporaryFile(fileName, out tempFileName); + // Create truncates where OpenWrite left a longer file's tail behind the body. + // Rewriting in place keeps the destination's own permissions and links. + FileStream fileStream = new( + fileName, + FileMode.Create, + FileAccess.Write, + FileShare.None + ); await using (fileStream.ConfigureAwait(false)) { await httpStream @@ -158,14 +160,9 @@ await httpStream .ConfigureAwait(false); } } - - // The destination is replaced only once the whole body is on disk. - // A download that fails partway therefore leaves what was already there untouched. - File.Move(tempFileName, fileName, overwrite: true); } catch (Exception e) when (Log.LogAndHandle(e)) { - DeleteTemporary(tempFileName); return false; } @@ -234,64 +231,6 @@ public static bool DownloadString(Uri uri, out string value) )] public static HttpClient GetHttpClient() => s_httpClient.Value; - [System.Diagnostics.CodeAnalysis.SuppressMessage( - "Reliability", - "CA2000:Dispose objects before losing scope", - Justification = "Ownership of the stream transfers to the caller, which disposes it." - )] - private static FileStream CreateTemporaryFile(string fileName, out string tempFileName) - { - // The temporary file sits beside the destination, so the replacing move stays on one volume. - string? directory = Path.GetDirectoryName(fileName); - string parent = string.IsNullOrEmpty(directory) ? "." : directory; - - for (int attempt = 0; ; attempt++) - { - string candidate = Path.Combine(parent, Path.GetRandomFileName()); - try - { - // CreateNew rather than Create, so a name already taken is never overwritten. - FileStream stream = new( - candidate, - FileMode.CreateNew, - FileAccess.Write, - FileShare.None - ); - - // Assigned only here, so the caller deletes a path this call actually created. - tempFileName = candidate; - return stream; - } - catch (IOException) when (attempt < TemporaryFileAttempts && File.Exists(candidate)) - { - // A random name collided, and another name is the whole remedy. - // Any other IO failure, a missing directory among them, is the caller's to report. - } - } - } - - private static void DeleteTemporary(string fileName) - { - // Empty means no temporary file was created, so there is nothing of this call's to remove. - if (string.IsNullOrEmpty(fileName)) - { - return; - } - - try - { - File.Delete(fileName); - } - catch (IOException) - { - // A partial download left behind is not itself a download failure. - } - catch (UnauthorizedAccessException) - { - // Same, where the file or its directory denies the delete. - } - } - /// /// Creates a URI with optional username and password credentials. /// diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index cfe19ee..4ff70ee 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -98,7 +98,7 @@ await File.WriteAllTextAsync( } [Fact] - public async Task DownloadFileAsync_WhenTheDownloadFails_ShouldLeaveTheDestination() + public async Task DownloadFileAsync_WhenTheRequestFails_ShouldLeaveTheDestination() { using LoopbackServer server = new(); string tempFile = Path.GetTempFileName(); @@ -116,7 +116,8 @@ public async Task DownloadFileAsync_WhenTheDownloadFails_ShouldLeaveTheDestinati _ = result.Should().BeFalse(); - // Truncating the destination at open time would leave an empty file here. + // The destination is opened only once the response is accepted. + // A request that never gets that far leaves it alone. string written = await File.ReadAllTextAsync( tempFile, TestContext.Current.CancellationToken @@ -142,8 +143,7 @@ public async Task DownloadFileAsync_WithAnUnusableDestination_ShouldReturnFalse( "file.txt" ); - // Naming the temporary file is part of the download. - // A destination that cannot hold one reports failure rather than throwing. + // A destination that cannot hold the file reports failure rather than throwing. bool result = await Download.DownloadFileAsync( server.OkUri, missingDirectory, @@ -151,7 +151,10 @@ public async Task DownloadFileAsync_WithAnUnusableDestination_ShouldReturnFalse( ); _ = result.Should().BeFalse(); - _ = File.Exists(missingDirectory).Should().BeFalse(); + + // The body is fetched before the destination is opened. + // The served request is what proves the failure came from the file rather than the request. + _ = server.RequestCount.Should().Be(1); } [Fact] diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index 40b25c7..b7e12bb 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -36,7 +36,7 @@ public void DownloadFile_OverALongerExistingFile_ShouldReplaceIt() } [Fact] - public void DownloadFile_WhenTheDownloadFails_ShouldLeaveTheDestination() + public void DownloadFile_WhenTheRequestFails_ShouldLeaveTheDestination() { using LoopbackServer server = new(); string tempFile = Path.GetTempFileName(); @@ -48,7 +48,8 @@ public void DownloadFile_WhenTheDownloadFails_ShouldLeaveTheDestination() _ = Download.DownloadFile(server.MissingUri, tempFile).Should().BeFalse(); - // Truncating the destination at open time would leave an empty file here. + // The destination is opened only once the response is accepted. + // A request that never gets that far leaves it alone. _ = File.ReadAllText(tempFile).Should().Be(existing); } finally @@ -70,10 +71,12 @@ public void DownloadFile_WithAnUnusableDestination_ShouldReturnFalse() "file.txt" ); - // Naming the temporary file is part of the download. - // A destination that cannot hold one reports failure rather than throwing. + // A destination that cannot hold the file reports failure rather than throwing. _ = Download.DownloadFile(server.OkUri, missingDirectory).Should().BeFalse(); - _ = File.Exists(missingDirectory).Should().BeFalse(); + + // The body is fetched before the destination is opened. + // The served request is what proves the failure came from the file rather than the request. + _ = server.RequestCount.Should().Be(1); } [Fact] From 3a6dde5ade15bc7859bb4be7575e880ef578a2c0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 07:21:18 -0700 Subject: [PATCH 08/11] Escape the Unicode Test Literal So the Source Stays ASCII The prose gate run over whole files, rather than over changed lines, reports seven unclassified non-ASCII characters in AppendLine_UnicodeCharacters_ShouldPreserve: four CJK ideographs and three astral-plane emoji, sitting literally in the source. The test's subject is that a non-ASCII string round-trips, so the string stays and the source loses the characters instead. The escapes compile to the identical string, verified against the original bytes, so the assertion is unchanged. Found by running the gate without --diff over every file this branch edits. The gate's own help states the policy the default hides: --diff "matches the repo policy: fix as each file is next edited, not swept", so a violation on an unchanged line belongs to whoever next edits that file. --- UtilitiesTests/StringHistoryTests.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/UtilitiesTests/StringHistoryTests.cs b/UtilitiesTests/StringHistoryTests.cs index 5950cd1..6893f9f 100644 --- a/UtilitiesTests/StringHistoryTests.cs +++ b/UtilitiesTests/StringHistoryTests.cs @@ -222,7 +222,10 @@ public void AppendLine_SpecialCharacters_ShouldPreserve() public void AppendLine_UnicodeCharacters_ShouldPreserve() { StringHistory history = new(); - string unicodeLine = "Unicode: δ½ ε₯½δΈ–η•Œ 🌍🌎🌏"; + + // Escaped rather than literal, so the source stays ASCII while the string does not. + // Four CJK ideographs and three astral-plane emoji, the latter being surrogate pairs. + string unicodeLine = "Unicode: \u4F60\u597D\u4E16\u754C \U0001F30D\U0001F30E\U0001F30F"; history.AppendLine(unicodeLine); From 6f7375583183f4ea17c7c3c254e4b547714a1906 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 07:22:56 -0700 Subject: [PATCH 09/11] Raise the Version Floor to 4.1 and Record the v4.1 Release Notes version.json carries the Nerdbank.GitVersioning floor, raised on develop so the promotion carries it to main, per OPERATIONS.md "Bumping the version floor". HISTORY.md gains a v4.1 section holding the two library fixes this branch ships, moved out of v4.0 where they were first written: the Download destination corruption and the StringHistory limit contract. The offline download tests and the coordination-reference removal stay out of both files, since neither ships in the package. README.md's release notes drop the Breaking Changes and Summary headings for one short line per fix under Version 4.1. That also drops four v4.0-era lines, including the only place the README named HttpClientFactory; HISTORY.md still carries all of it under v4.0. Method names in both files now end in (), so a method reads as one rather than as a property, and the four v4.0-era instances that predate this change are corrected with them. The StringHistory line no longer reads as though a negative value is accepted, which is what it said while listing the defects being fixed. Five prose semicolons in shipped v4.0 entries go with them, found by running the prose gate over whole files rather than changed lines. --- HISTORY.md | 19 ++++++++++--------- README.md | 15 +++------------ version.json | 2 +- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 99ad38e..0ea3c1f 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -4,20 +4,21 @@ Some useful and not so useful C# .NET utility classes. ## Release History +- v4.1: + - Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination file: both opened it with `File.OpenWrite()`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now truncate and rewrite the destination in place, which keeps its permissions, ownership, and any links to it, and open it only once the response is accepted, so a request that fails before that leaves it untouched. + - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later `AppendLine()`, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. `AppendLine()` changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. - v4.0: - - Fixed `Download.DownloadFile` and `DownloadFileAsync` corrupting the destination file: both opened it with `File.OpenWrite`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now write beside the destination and move the result into place, so the destination is replaced whole on success and left untouched when a download fails partway. - - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later append, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. Appending changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. - - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`; it exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. + - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`. It exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. - Added `AssemblyInfo`, an AOT safe assembly and application identity helper whose `For()` substitutes for `Assembly.GetExecutingAssembly()` (unreliable under Native AOT), and which supplies the consuming application name, version, and a default User-Agent. - - Reworked `Download` to build its `HttpClient` through `HttpClientFactory`, so downloads now flow through the shared retry and circuit-breaker pipeline; the `TimeoutSeconds` property and all method signatures are unchanged. - - Changed public members that exposed `List` to safe collection types (a breaking API change): `FileEx.EnumerateDirectories` / `EnumerateDirectory` now return `Collection` out parameters and accept `IEnumerable`, and `StringHistory.StringList` is now a `ReadOnlyCollection`. + - Reworked `Download` to build its `HttpClient` through `HttpClientFactory`, so downloads now flow through the shared retry and circuit-breaker pipeline. The `TimeoutSeconds` property and all method signatures are unchanged. + - Changed public members that exposed `List` to safe collection types (a breaking API change): `FileEx.EnumerateDirectories()` / `EnumerateDirectory()` now return `Collection` out parameters and accept `IEnumerable`, and `StringHistory.StringList` is now a `ReadOnlyCollection`. - Gated the library's reference AOT verification behind an explicit `PublishAot` opt-in, and turned the `Sandbox` project into a Native AOT smoke test (published and run as AOT) that proves the resilience pipeline and assembly-identity resolution work under Native AOT. - - Renamed the NuGet package and root namespace from `InsaneGenius.Utilities` to `ptr727.Utilities` (a breaking change); consumers must update their package reference and change `using InsaneGenius.Utilities;` directives to `using ptr727.Utilities;`. The assembly is now named `Utilities`. + - Renamed the NuGet package and root namespace from `InsaneGenius.Utilities` to `ptr727.Utilities`, a breaking change. Consumers must update their package reference and change `using InsaneGenius.Utilities;` directives to `using ptr727.Utilities;`. The assembly is now named `Utilities`. - Replaced the Serilog-coupled logging model with the backend-agnostic `Microsoft.Extensions.Logging` abstraction. - - Removed the global Serilog `LogOptions.Logger` property (a breaking API change) in favor of a thread-safe, injectable `ILoggerFactory` configured via `LogOptions.SetFactory(...)` / `TrySetFactory(...)`; the library now depends only on `Microsoft.Extensions.Logging.Abstractions`. + - Removed the global Serilog `LogOptions.Logger` property (a breaking API change) in favor of a thread-safe, injectable `ILoggerFactory` configured via `LogOptions.SetFactory(...)` / `TrySetFactory(...)`. The library now depends only on `Microsoft.Extensions.Logging.Abstractions`. - Reworked `FileEx` and `Download` to resolve per-class cached loggers through `LogOptions.CreateLogger(...)` and to emit source-generated `[LoggerMessage]` messages, keeping the build clean under `AnalysisMode=All` and `TreatWarningsAsErrors`. - - Moved the `LogAndHandle` / `LogAndPropagate` helpers onto `Microsoft.Extensions.Logging.ILogger` as internal extensions (exposed to tests via `InternalsVisibleTo`). - - Renamed the public `Extensions` class to `CompressExtensions` (a breaking API change for direct references; instance-style extension calls such as `value.Compress()` are unaffected), resolving the naming clash with the `Microsoft.Extensions` namespace. + - Moved the `LogAndHandle()` / `LogAndPropagate()` helpers onto `Microsoft.Extensions.Logging.ILogger` as internal extensions (exposed to tests via `InternalsVisibleTo`). + - Renamed the public `Extensions` class to `CompressExtensions` (a breaking API change for direct references, though instance-style extension calls such as `value.Compress()` are unaffected), resolving the naming clash with the `Microsoft.Extensions` namespace. - Updated the `Sandbox` example to configure a Serilog console logger and inject it through a `SerilogLoggerFactory`. - Dropped the library's Serilog dependency and its `IL3058` AOT warning suppression. - v3.6: diff --git a/README.md b/README.md index f181e2d..0cd2640 100644 --- a/README.md +++ b/README.md @@ -22,19 +22,10 @@ Some useful and not so useful C# .NET utility classes. ### Release Notes -**Version: 4.0**: +**Version: 4.1**: -**⚠️ Breaking Changes**: - -- Renamed the NuGet package and namespace from `InsaneGenius.Utilities` to `ptr727.Utilities`. -- Replaced Serilog with an injectable `Microsoft.Extensions.Logging` logging model. -- Replaced `List` in public methods with `Collection` and `ReadOnlyCollection`. -- Changed `StringHistory.MaxFirstLines` and `MaxLastLines` to re-partition the lines already stored when assigned, and to reject a negative value. - -**Summary**: - -- Added `HttpClientFactory`, a reusable resilient HTTP client factory (Polly retry and circuit breaker) with an AOT safe `AssemblyInfo` identity helper and a tunable `HttpClientOptions`. -- Fixed `Download.DownloadFile` and `DownloadFileAsync` corrupting the destination when downloading over a longer existing file. The destination is now replaced whole on success and left untouched on failure. +- Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination when downloading over a longer existing file. The destination is now truncated and rewritten in place, keeping its permissions and any links to it. +- Fixed the `StringHistory` limit properties: `MaxFirstLines` and `MaxLastLines` now honor a limit assigned after lines have been appended, document zero as retaining no lines on that side rather than as no limit, and reject a negative value with `ArgumentOutOfRangeException`. See [Release History](./HISTORY.md) for complete release notes and older versions. diff --git a/version.json b/version.json index e9df9b7..6897883 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json", - "version": "4.0", + "version": "4.1", "publicReleaseRefSpec": [ "^refs/heads/main$" ], From aabeb0c4b45d72a4529dfd350d1f371f363bc5b7 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 07:33:32 -0700 Subject: [PATCH 10/11] Correct What the Release Notes and the Contract Claim About a Download A local pass over the release notes found three claims that did not match the code, and one behavior difference the notes did not mention at all. HISTORY.md credited v4.1 with opening the destination only once the response is accepted, so a failed request leaves it untouched. v4.0 already did that: it called GetStreamAsync before File.OpenWrite, so a request that failed never reached the file. The entry claimed unchanged behavior as a fix. The same entry omitted what did change. Truncating at open means a download that fails partway now leaves a short file, where OpenWrite left the original bytes behind the newly written ones. That is a real loss of content for a consumer whose download is interrupted, it is the cost of the fix rather than a detail, and HISTORY.md is where a consumer would look for it. Both it and README.md now say so. Windows refuses FileMode.Create against a hidden file, where the FileMode.OpenOrCreate that OpenWrite used does not, so a download over a hidden destination would have started failing on a supported platform, contradicting the claim that the destination's own attributes survive. Both methods now open with OpenOrCreate and truncate with SetLength, which is what fixes the original defect without changing which destinations can be opened. Re-probed on Linux: mode 0600 survives with the file truncated to the body's length, a hard link sees the new content, a symlink is written through, and a read-only destination is still refused with the file intact. The Windows half is documented behavior rather than something this host can run. ARCHITECTURE.md named a public type Extensions that v4.0 renamed to CompressExtensions, so the contract list described a type consumers cannot reference, and it attributed the logger helpers to it rather than to the internal LogExtensions. It also named FileEx's options as Options rather than the static FileEx.Options, and wrote its own method names without the parentheses the same change had just standardized on in HISTORY.md and README.md. Two test comments said the body is fetched before the destination is opened. GetStreamAsync completes on the response headers, so only those have been read and the body streams after the file is open. --- ARCHITECTURE.md | 6 +++--- HISTORY.md | 2 +- README.md | 2 +- Utilities/Download.cs | 17 +++++++++++------ UtilitiesTests/DownloadAsyncTests.cs | 2 +- UtilitiesTests/DownloadTests.cs | 2 +- 6 files changed, 18 insertions(+), 13 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index cf61938..3268049 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -18,9 +18,9 @@ These are behavioral contracts rather than formatting rules, which is why they l - **I/O methods return `bool`** for success or failure, and hand back any additional result through an `out` parameter. They do not signal ordinary I/O failure by throwing. - **Async methods carry the `Async` suffix** and take an optional `CancellationToken cancellationToken = default`, passed through to the underlying call rather than ignored. An async overload returning several values returns a tuple, since `out` parameters are unavailable there. -- **`Download`** reuses a thread-safe `Lazy`. `GetContentInfo` reads with `HttpCompletionOption.ResponseHeadersRead`, so asking for a size never fetches the body, while `DownloadString` buffers the whole response and a large body belongs in `DownloadFile` instead. A download to a file truncates and rewrites the destination in place, so its permissions, ownership, and any links to it survive the download, and a request that fails before the response is accepted leaves it untouched. The destination is opened once the response is accepted, so a failure during the body can leave a partial file. -- **`FileEx`** wraps its I/O in retry logic configured through `Options`, and honors cancellation from both `Options.Cancel` and the method's own token parameter. +- **`Download`** reuses a thread-safe `Lazy`. `GetContentInfo()` reads with `HttpCompletionOption.ResponseHeadersRead`, so asking for a size never fetches the body, while `DownloadString()` buffers the whole response and a large body belongs in `DownloadFile()` instead. A download to a file truncates and rewrites the destination in place, so its permissions, ownership, and any links to it survive. The destination is truncated once the response headers are accepted rather than once the body has arrived, so a request that fails before that leaves it untouched, while one that fails partway through the body leaves a short file. +- **`FileEx`** wraps its I/O in retry logic configured through the static `FileEx.Options`, a `FileExOptions`, and honors cancellation from both `FileEx.Options.Cancel` and the method's own token parameter. - **`StringCompression`** uses Deflate, takes a configurable compression level, and passes `leaveOpen` so the caller keeps ownership of the stream it supplied. - **`StringHistory`** retains at most `MaxFirstLines` from the head and `MaxLastLines` from the tail. Zero on a single side retains no lines on that side, and zero on both is the one unrestricted mode. Either limit rejects a negative value, and assigning one re-partitions the lines already stored, so the history never holds more than the limits then in force allow. Re-partitioning only discards: once a line has been dropped the head is closed, so a later, larger `MaxFirstLines` never promotes a retained tail line into it. -- **`Extensions`** uses the C# `extension` block form inside a static class for its logger and string helpers. +- **`CompressExtensions`** uses the C# `extension` block form inside a static class for its string helpers, and the internal `LogExtensions` does the same for the logger helpers. - **Logging is a seam, never a dependency.** The library depends on `Microsoft.Extensions.Logging.Abstractions` and takes an `ILoggerFactory` through `LogOptions`. It references no logging framework or sink, so a consumer chooses its own. `Serilog` appears only in `Sandbox` and the tests, where an application legitimately picks one. diff --git a/HISTORY.md b/HISTORY.md index 0ea3c1f..e3443f3 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,7 +5,7 @@ Some useful and not so useful C# .NET utility classes. ## Release History - v4.1: - - Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination file: both opened it with `File.OpenWrite()`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now truncate and rewrite the destination in place, which keeps its permissions, ownership, and any links to it, and open it only once the response is accepted, so a request that fails before that leaves it untouched. + - Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination file: both opened it with `File.OpenWrite()`, which does not truncate, so a download over a longer existing file left that file's trailing bytes after the downloaded content and reported success. Both now truncate the destination explicitly and rewrite it in place, which keeps its permissions, ownership, and any links to it. The truncation happens once the response headers are accepted rather than once the body has arrived, so a download that fails partway now leaves a short file where it previously left the original bytes behind the new ones. - Tightened the `StringHistory` limit contract: `MaxFirstLines` and `MaxLastLines` now document zero as retaining no lines on that side rather than as no limit (both at zero remains the unrestricted mode), reject a negative value with `ArgumentOutOfRangeException` at the constructor and at the property rather than at a later `AppendLine()`, and re-partition the lines already stored when assigned, so a limit set after appending is honored instead of ignored. `AppendLine()` changed with them: what is retained is now always a prefix of the appended lines followed by a suffix of them, so once a line has been discarded the head is trimmed but never refilled, and a later, larger `MaxFirstLines` raises the ceiling without adopting retained tail lines as first lines. - v4.0: - Added `HttpClientFactory`, a reusable resilient HTTP client factory built on `Microsoft.Extensions.Http.Resilience` (Polly) with retry, circuit breaker, and connection pooling, tunable through the new `HttpClientOptions`. It exposes a shared singleton client, caller-owned clients, and the resilience handler for callers that build their own client with a custom base address or headers. diff --git a/README.md b/README.md index 0cd2640..d4f463e 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ Some useful and not so useful C# .NET utility classes. **Version: 4.1**: -- Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination when downloading over a longer existing file. The destination is now truncated and rewritten in place, keeping its permissions and any links to it. +- Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination when downloading over a longer existing file. The destination is now truncated and rewritten in place, keeping its permissions and any links to it. A download that fails partway leaves a short file rather than a mix of the new content and the old. - Fixed the `StringHistory` limit properties: `MaxFirstLines` and `MaxLastLines` now honor a limit assigned after lines have been appended, document zero as retaining no lines on that side rather than as no limit, and reject a negative value with `ArgumentOutOfRangeException`. See [Release History](./HISTORY.md) for complete release notes and older versions. diff --git a/Utilities/Download.cs b/Utilities/Download.cs index b8ba349..04c5a82 100644 --- a/Utilities/Download.cs +++ b/Utilities/Download.cs @@ -103,14 +103,17 @@ public static bool DownloadFile(Uri uri, string fileName) { using Stream httpStream = GetHttpClient().GetStreamAsync(uri).GetAwaiter().GetResult(); - // Create truncates where OpenWrite left a longer file's tail behind the body. - // Rewriting in place keeps the destination's own permissions and links. + // Rewriting in place keeps the destination's own permissions, ownership, and links. + // OpenOrCreate rather than Create, which on Windows refuses a hidden destination. using FileStream fileStream = new( fileName, - FileMode.Create, + FileMode.OpenOrCreate, FileAccess.Write, FileShare.None ); + + // Truncate explicitly, which OpenWrite never did, leaving a longer file's tail behind. + fileStream.SetLength(0); httpStream.CopyTo(fileStream); } catch (Exception e) when (Log.LogAndHandle(e)) @@ -145,16 +148,18 @@ public static async Task DownloadFileAsync( .ConfigureAwait(false); await using (httpStream.ConfigureAwait(false)) { - // Create truncates where OpenWrite left a longer file's tail behind the body. - // Rewriting in place keeps the destination's own permissions and links. + // Rewriting in place keeps the destination's permissions, ownership, and links. + // OpenOrCreate rather than Create, which on Windows refuses a hidden destination. FileStream fileStream = new( fileName, - FileMode.Create, + FileMode.OpenOrCreate, FileAccess.Write, FileShare.None ); await using (fileStream.ConfigureAwait(false)) { + // Truncate explicitly, which OpenWrite never did, leaving a longer file's tail. + fileStream.SetLength(0); await httpStream .CopyToAsync(fileStream, cancellationToken) .ConfigureAwait(false); diff --git a/UtilitiesTests/DownloadAsyncTests.cs b/UtilitiesTests/DownloadAsyncTests.cs index 4ff70ee..db67fa2 100644 --- a/UtilitiesTests/DownloadAsyncTests.cs +++ b/UtilitiesTests/DownloadAsyncTests.cs @@ -152,7 +152,7 @@ public async Task DownloadFileAsync_WithAnUnusableDestination_ShouldReturnFalse( _ = result.Should().BeFalse(); - // The body is fetched before the destination is opened. + // The response headers are read before the destination is opened. // The served request is what proves the failure came from the file rather than the request. _ = server.RequestCount.Should().Be(1); } diff --git a/UtilitiesTests/DownloadTests.cs b/UtilitiesTests/DownloadTests.cs index b7e12bb..1152fec 100644 --- a/UtilitiesTests/DownloadTests.cs +++ b/UtilitiesTests/DownloadTests.cs @@ -74,7 +74,7 @@ public void DownloadFile_WithAnUnusableDestination_ShouldReturnFalse() // A destination that cannot hold the file reports failure rather than throwing. _ = Download.DownloadFile(server.OkUri, missingDirectory).Should().BeFalse(); - // The body is fetched before the destination is opened. + // The response headers are read before the destination is opened. // The served request is what proves the failure came from the file rather than the request. _ = server.RequestCount.Should().Be(1); } From 097ee75bfa0cd90da640014f173ac88f3f156d4f Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 1 Sep 2026 07:52:40 -0700 Subject: [PATCH 11/11] State the Both-Zero Exception in the README Limit Summary The README said zero retains no lines on that side, without the exception that ARCHITECTURE.md and HISTORY.md both carry: zero on both sides at once is the one unrestricted mode. Read alone, the short summary described a StringHistory that retains nothing by default, which is the opposite of what a default-constructed one does. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index d4f463e..68c77fb 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ Some useful and not so useful C# .NET utility classes. **Version: 4.1**: - Fixed `Download.DownloadFile()` and `DownloadFileAsync()` corrupting the destination when downloading over a longer existing file. The destination is now truncated and rewritten in place, keeping its permissions and any links to it. A download that fails partway leaves a short file rather than a mix of the new content and the old. -- Fixed the `StringHistory` limit properties: `MaxFirstLines` and `MaxLastLines` now honor a limit assigned after lines have been appended, document zero as retaining no lines on that side rather than as no limit, and reject a negative value with `ArgumentOutOfRangeException`. +- Fixed the `StringHistory` limit properties: `MaxFirstLines` and `MaxLastLines` now honor a limit assigned after lines have been appended, document zero on one side as retaining no lines on that side rather than as no limit, with both at zero remaining the one unrestricted mode, and reject a negative value with `ArgumentOutOfRangeException`. See [Release History](./HISTORY.md) for complete release notes and older versions.