diff --git a/CHANGELOG.md b/CHANGELOG.md index b004ba9..448bdc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,11 @@ rather than deprecated, so consuming code must be updated. - **`ColorTemperature.Kelvin` range widened to `1000–20000 K`** to match the docs' forward- compatible guidance, with a new `ColorTemperature.KelvinUnchecked(int)` escape hatch for values outside that range. +- **`WLedClient(string)` now refreshes DNS.** The convenience constructor's self-owned + `HttpClient` uses a `SocketsHttpHandler` with a bounded `PooledConnectionLifetime` (2 minutes) + on modern runtimes, so a long-lived client picks up DNS/IP changes instead of pinning a stale + connection (fixes [#8](https://github.com/kevbite/WLED.NET/issues/8)). The `netstandard2.0` + build falls back to `HttpClientHandler`; use `IHttpClientFactory`/DI there. ### Added diff --git a/README.md b/README.md index eec2ee2..344a33b 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,11 @@ services.AddWledClient("http://office-computer-wled/"); services.AddWledClient(client => client.BaseAddress = new Uri("http://office-computer-wled/")); ``` +The `WLedClient(string)` constructor owns its `HttpClient` and uses a `SocketsHttpHandler` +with a bounded `PooledConnectionLifetime`, so a long-lived client still picks up DNS changes +(e.g. a WLED device that gets a new IP). For applications, registering via +`IHttpClientFactory`/DI as above remains the recommended approach. + ### Quick commands Common operations have first-class "intent" methods: diff --git a/plans/14-httpclient-connection-lifetime.md b/plans/14-httpclient-connection-lifetime.md new file mode 100644 index 0000000..95a6636 --- /dev/null +++ b/plans/14-httpclient-connection-lifetime.md @@ -0,0 +1,111 @@ +# Plan 14 — HttpClient connection lifetime & DNS staleness + +**Theme:** Correctness · Reliability · Cross-cutting + +Tracks [issue #8](https://github.com/kevbite/WLED.NET/issues/8). + +## Why + +A long-lived `HttpClient` over a default `HttpClientHandler` keeps its TCP connections +open indefinitely and therefore **never honours DNS changes** — a documented .NET pitfall +(see [HttpClient guidelines: DNS behavior](https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines#dns-behavior)). + +WLED devices are a realistic trigger for this: + +- they are frequently addressed by mDNS/hostname or a DHCP-assigned IP that can change + (device reboot, lease renewal, network change), and +- a typical consumer creates **one** `WLedClient("http://wled.local/")` for the lifetime of + the app and reuses it. + +The DI path (`services.AddWledClient(...)`) is already safe: it goes through +`IHttpClientFactory`, which rotates handlers on a default 2-minute lifetime. The problem is +isolated to the convenience constructors that own a self-created `HttpClient`: + +- `WLedClient(string baseUri)` → `new HttpClientHandler()` (see `WLedClient.cs`). +- `WLedClient(HttpMessageHandler, string)` → caller-supplied handler (out of scope; the + caller owns its lifetime). + +## Goal + +When the library creates the handler itself, give pooled connections a finite lifetime so +DNS is periodically refreshed, **without** regressing throughput or the netstandard2.0 +target. + +## Approach + +In `WLedClient`, replace the default `new HttpClientHandler()` used by +`WLedClient(string baseUri)` with a `SocketsHttpHandler` configured with a bounded +`PooledConnectionLifetime`, on runtimes where `SocketsHttpHandler` exists. + +`SocketsHttpHandler` is available on .NET Core 2.1+/.NET 5+ but **not** in the +netstandard2.0 reference assemblies, so this must be guarded by a target-framework +conditional rather than a runtime check: + +```csharp +private static HttpMessageHandler CreateDefaultHandler() +{ +#if NETSTANDARD2_0 + // .NET Framework / legacy: SocketsHttpHandler is unavailable. Fall back to the + // platform handler; consumers on these runtimes should prefer IHttpClientFactory. + return new HttpClientHandler(); +#else + return new SocketsHttpHandler + { + // Refresh pooled connections (and therefore DNS) periodically. + PooledConnectionLifetime = TimeSpan.FromMinutes(2) + }; +#endif +} +``` + +Wire it into the existing constructor: + +```csharp +public WLedClient(string baseUri) : this(CreateDefaultHandler(), baseUri) +{ +} +``` + +Notes / decisions: + +- **Two minutes** mirrors `IHttpClientFactory`'s default handler lifetime, keeping the two + paths consistent. It is a reasonable default for LAN devices and can be revisited if a + configurable overload is later requested. +- Keep the existing `Connection: keep-alive` default request header; `PooledConnectionLifetime` + bounds connection reuse without disabling keep-alive. +- **Do not** change the `WLedClient(HttpMessageHandler, string)` or `WLedClient(HttpClient)` + constructors — those hand ownership of the handler/client to the caller (including DI), + and overriding their lifetime would be surprising. +- netstandard2.0 constraint: `SocketsHttpHandler` is not referenced under that TFM, so the + `#if` keeps the netstandard2.0 build green (it currently builds clean and must stay so). + +## Tests + +- A test asserting `new WLedClient("http://wled.local/")` constructs successfully and can + issue a request (the existing `MockHttpMessageHandler` path already covers request + behaviour; this guards the default-handler wiring on net8/9/10). +- Because handler internals aren't observable through `HttpClient`, prefer a small, + internal, testable seam if exact `PooledConnectionLifetime` verification is wanted: + e.g. an `internal static HttpMessageHandler CreateDefaultHandler()` exposed to the test + project via `InternalsVisibleTo`, asserting it returns a `SocketsHttpHandler` with the + expected lifetime on modern TFMs. Keep this minimal and conditional-compiled. +- Confirm the full multi-target build (incl. netstandard2.0) and the existing 205 tests + remain green. + +## Definition of Done (per `plans/README.md`) + +1. **README.md** — add a short note under connecting/DI guidance explaining that the + convenience constructor now refreshes DNS via a bounded connection lifetime, and that + `IHttpClientFactory`/DI remains the recommended approach for apps. +2. **samples/BasicConsole** — no behavioural change required; the existing + `new WLedClient(...)` usage now benefits automatically. Update a comment if helpful. +3. **CHANGELOG.md** — record the fix under the unreleased section + (e.g. *"The `WLedClient(string)` constructor now uses a `SocketsHttpHandler` with a + bounded `PooledConnectionLifetime` so DNS changes are picked up (fixes #8)."*). + +## Out of scope + +- Making the connection lifetime configurable via a new constructor/options overload — can + be a fast follow if requested. +- Any change to the DI package, which already delegates lifetime management to + `IHttpClientFactory`. diff --git a/plans/README.md b/plans/README.md index a847a8d..d9617a9 100644 --- a/plans/README.md +++ b/plans/README.md @@ -66,6 +66,7 @@ These plans cross-reference two mature community libraries (linked by the WLED d | 11 | [Client ergonomics & cross-cutting concerns](11-client-ergonomics-and-cross-cutting.md) | Quality | | 12 | [Usability & correctness improvements (post-review)](12-usability-and-correctness-improvements.md) | Correctness | | 13 | [Device ergonomics, catalogs & agent guidance](13-device-ergonomics-and-agent-guidance.md) | Usability | +| 14 | [HttpClient connection lifetime & DNS staleness](14-httpclient-connection-lifetime.md) | Correctness | Plan 0 modernises the toolchain (multi-targeting `netstandard2.0;net8.0;net9.0;net10.0`) and should land first. Plans 1–2 are the ergonomic foundation and unblock everything else. Plans 3–10 add diff --git a/src/Kevsoft.WLED/Kevsoft.WLED.csproj b/src/Kevsoft.WLED/Kevsoft.WLED.csproj index d653112..8d7edcf 100644 --- a/src/Kevsoft.WLED/Kevsoft.WLED.csproj +++ b/src/Kevsoft.WLED/Kevsoft.WLED.csproj @@ -22,6 +22,10 @@ + + + + diff --git a/src/Kevsoft.WLED/WLedClient.cs b/src/Kevsoft.WLED/WLedClient.cs index 9dc51f7..eb406ee 100644 --- a/src/Kevsoft.WLED/WLedClient.cs +++ b/src/Kevsoft.WLED/WLedClient.cs @@ -9,7 +9,7 @@ public WLedClient(HttpMessageHandler httpMessageHandler, string baseUri) { } - public WLedClient(string baseUri) : this(new HttpClientHandler(), baseUri) + public WLedClient(string baseUri) : this(CreateDefaultHandler(), baseUri) { } @@ -38,6 +38,25 @@ private static HttpClient CreateClient(HttpMessageHandler httpMessageHandler, st return client; } + /// + /// Creates the default used when the client owns its own + /// . On modern runtimes this is a SocketsHttpHandler with a bounded + /// PooledConnectionLifetime so pooled connections — and therefore DNS — are refreshed + /// periodically. See + /// https://learn.microsoft.com/dotnet/fundamentals/networking/http/httpclient-guidelines#dns-behavior. + /// + internal static HttpMessageHandler CreateDefaultHandler() + { +#if NETSTANDARD2_0 + return new HttpClientHandler(); +#else + return new SocketsHttpHandler + { + PooledConnectionLifetime = TimeSpan.FromMinutes(2) + }; +#endif + } + public Task Get(CancellationToken cancellationToken = default) => GetJson("json", cancellationToken); diff --git a/test/Kevsoft.WLED.Tests/WLedClientConstructionTests.cs b/test/Kevsoft.WLED.Tests/WLedClientConstructionTests.cs new file mode 100644 index 0000000..7654e59 --- /dev/null +++ b/test/Kevsoft.WLED.Tests/WLedClientConstructionTests.cs @@ -0,0 +1,25 @@ +using System.Net.Http; + +namespace Kevsoft.WLED.Tests; + +public class WLedClientConstructionTests +{ + [Fact] + public void StringConstructorSucceeds() + { + var act = () => new WLedClient("http://wled.local/"); + + act.Should().NotThrow(); + } + +#if !NETSTANDARD2_0 + [Fact] + public void DefaultHandlerRefreshesPooledConnectionsToAvoidStaleDns() + { + var handler = WLedClient.CreateDefaultHandler(); + + handler.Should().BeOfType() + .Which.PooledConnectionLifetime.Should().Be(TimeSpan.FromMinutes(2)); + } +#endif +}