diff --git a/SESSION_LOG.md b/SESSION_LOG.md index 8c105f8..d653612 100644 --- a/SESSION_LOG.md +++ b/SESSION_LOG.md @@ -40,3 +40,41 @@ - BENCH (real Nq1, fw 3.7.2, USB, non-destructive) via a scratch harness, because the example CLI surfaces none of these counters. **Session 1 (ch 0,1,2 @200 Hz, 3 s):** `rawFrames=475` and `decodedCh0=475` — every delivered frame reached BOTH consumer paths exactly once (the re-raise multiplicity contract, on hardware); `discarded=1` with `PartialAnalogFrame[an=1/en=3]` — the firmware's malformed leading frame caught by the moved guard and withheld from raw consumers; all 3 channels decoded 475 samples each in ascending order; `decodeFailures=0`, `gaps=0`; every event's `sender` asserted to be the device. **Session 2 (ch 0 only @100 Hz, same instance):** `discarded=0`/`failures=0` (BeginSession reset both, no leftover tripped the gate); `rawFrames=238` vs `decodedCh0=237` — one post-stop frame re-raised but NOT decoded, exactly the `if (!IsStreaming)` branch on hardware; `ch1=ch2=0` decoded, so the disable reached the device and the snapshot the decode maps against is still right. Only channel enable/disable + stream start/stop; no NVM write, no reboot, no SD. - Bench-rig note: the example CLI's `--channels` takes a **bitmask** (`7` = ch 0,1,2), not a comma list, and `--format` accepts only `text|csv|jsonl` (no `json`). - Result: PR #435 opened (base main, part of #344, "not merging — for review"), /agentic_review requested. Now **3 loop PRs awaiting review (#433, #434, #435) — at the concurrency cap**, so the next fire should shepherd only, not start a new ticket. + +## 2026-08-05 — Fire: PR #436 ready-note + implemented #344 (lifecycle-gate extraction) → PR #437 +- State at start (re-derived from `gh`, NOT this log — the log's closing line said "at the cap with #433/#434/#435", which was stale on arrival): #433 and #435 have MERGED, and a prior fire opened **#436** without writing an entry. Live open loop PRs were **#434 and #436 → 2, under the 3-PR cap**, so priority 4 was in scope. +- Priority 1: zero unresolved `qodo-code-review` threads on both (GraphQL `reviewThreads`, not the summary comment). Priority 2: #436 was Qodo-clean (0 bugs / 0 rule violations / 0 requirement gaps) + CI `build` green with **no** ready-note → posted one. #434 already had its note. Priority 3: no red CI. +- **Working-tree incident, recorded so it isn't repeated.** While surveying file sizes I ran `git checkout origin/main -- .` in the *shared* checkout at `/Users/tylerkron/projects/daqifi/daqifi-core` (which sits on `main` @ `7bc6161`, two commits behind origin). That staged the #433+#435 delta over the user's checkout. Verified the damage was recoverable before touching anything: `git diff --cached` was byte-identical to `git diff HEAD origin/main`, i.e. 100% upstream-committed content with no novel local work in it, then `git reset --hard 7bc6161` restored it exactly as found. **All loop work belongs in the fire's own worktree; never run a checkout/reset in the shared clone.** +- Backlog is still 5 open issues, 4 of them skips under the loop rules (#333 breaking API; #269/#271 destructive WINC flash; #183 unshipped firmware dep). #344 was again the only eligible one — and its two named god-classes are now largely done (`FirmwareUpdateService` 2,656 → 359 via #419; the SD/diagnostics/network/LAN blocks, channel control, frame decode all extracted). `DaqifiDevice.cs` at **4,193 lines is now the largest file in the repo**, which is item 4 of the issue's own status comment. +- CANDIDATE REJECTED — anything in `DaqifiStreamingDevice.cs` (still 1,385 lines, over the ~800 target): **PR #436 owns that file and `IDeviceOperationHost.cs` on an unmerged branch**, so it would conflict textually. `WifiModuleUpdater` (item 3) stays deferred until #271 settles the external-tool shape. +- PICKED the **lifecycle-serialization block** out of `DaqifiDevice` (issue #379's connect-vs-disconnect gate) → `Device/Internal/LifecycleGate.cs`. Chosen over the larger reconnection region (~500 lines) deliberately: the gate has **4 call sites, no events, and no device state at all**, where reconnection owns three events and two `protected virtual` hooks that `DaqifiStreamingDevice` overrides. Smallest safe bite, per the #433 precedent. `DaqifiDevice` 4,193 → 3,947. +- The seam is `(ILogger, Func deviceName, Func connect, Func teardown)`. **The timeouts are delegates and that is load-bearing, not stylistic**: `LifecycleLockTimeout`/`TeardownLockTimeout` stay `internal virtual` on the device precisely so tests can shorten them, and `DeviceReconnectTests`' subclass assigns its override through an `init` property that runs *after* the base constructor builds the gate. Capturing values there would have silently pinned the 10 s/30 s defaults and ignored every override. Pinned by its own test. +- Verified as a move mechanically, same method as #419/#422/#432/#433: normalized statement multiset diff, run in **both directions**. Collaborator vs the original region leaves only file scaffolding + the ctor/fields/null-guards + the `SafeLog` copy; device before-vs-after shows it lost exactly the region body and gained exactly the field, 3 constructor wirings and 4 call-site renames. Nothing unaccounted. +- `SafeLog` is a per-class private copy, which is the established convention here (`MessageProducer`, `DaqifiDevice`, and `StatusChannelPopulator` from #433 each carry their own) — not a new shared helper. +- Left behavior alone deliberately: `_lifecycleLock` is **never disposed** today (`ReleaseResources` disposes only `_textExchangeLock`), so the `ObjectDisposedException` handlers are defensive rather than reachable. Adding disposal would be a behavior change smuggled into a pure refactor; documented in the collaborator instead. +- Tests: **zero edits to existing tests** — the real evidence, since `DeviceReconnectTests` still races a live reconnect loop against a caller's connect through the new seam. +21 new cases against the collaborator directly, which the extraction newly makes possible (contention is now *constructed* rather than raced into): both policies × sync/async, re-entry incl. across an `await`, the re-entry flag not leaking, release-on-throw, the lazy-timeout seam, and the #341 cancelled-teardown contract. FULL suite green net9 + net10 (2,588 passed, 2 skipped each) + Daqifi.Mcp.Tests 23. Release solution build **0 warnings** both TFMs — the gate that catches a bad `` after moving ~280 lines of doc prose across a namespace. +- BENCH (real Nq1, fw 3.7.2, USB, non-destructive — connect/disconnect/status only, no stream start, no NVM write, no reboot): 3 sync connect/disconnect cycles on ONE instance (a leaked permit would make cycle 2 throw), 2 async cycles, and the two things a unit test genuinely cannot show — a cancelled `DisconnectAsync` **actually released the OS serial handle** (proved by a fresh device reopening the port, not by reading `Status`), and `Disconnect()` re-entered from inside a live `StatusChanged` handler completed instead of deadlocking against the real transport. Device healthy afterward: fw 3.7.2, analogIn=16 digital=16. +- Result: PR #437 opened (base main, part of #344, "not merging — for review"), `/agentic_review` requested. Touches only `DaqifiDevice.cs` + two new files — deliberately zero file overlap with #434 and #436. Now 3 loop PRs awaiting review (#434, #436, #437) — **at the cap**, so the next fire shepherds only until the user merges one. + +## 2026-08-05 — Fire: shepherded PR #437's Qodo review (1 finding, valid) +- State at start (re-derived from `gh`): 3 open loop PRs — **#434, #436, #437 — at the concurrency cap**, so priority 4 (new ticket) was out of scope by rule and this fire was shepherd-only. Backlog unchanged at 5 open issues (#344, #333, #271, #269, #183), 4 of them standing skips. +- Priority 1 sweep: #434 and #436 both Qodo-clean with 0 unresolved threads and ready-notes already posted — nothing to do. **#437 had been open 90 seconds**: CI `build` had just gone green but Qodo had only posted its PR Summary, so it was not yet actionable. Polled rather than declaring the fire idle; the review landed ~2.5 min later with **1 finding**. Priority 3: no red CI anywhere. +- FINDING (Optional/Informational, "No null-check operation delegate") — **VALID, fixed in `ff2e30b`**. `LifecycleGate`'s constructor null-guards all four injected delegates, but `Run`/`RunAsync` invoked the caller's `operation` unvalidated, so internal misuse would surface as `NullReferenceException` from one of three invocation sites instead of a named `ArgumentNullException`. Not a judgement call: the sibling collaborator `ChannelControlOperations` (extracted in #432, merged) already guards **every** entry point with `ArgumentNullException.ThrowIfNull` — the gate was inconsistent with a settled in-repo convention, not deliberately lenient. +- **Placement was the part the finding did not specify and the part that matters.** The guard goes *before* the re-entry branch, not merely before the invocation: `if (_isInsideLifecycleOperation.Value) { operation(); }` invokes without acquiring anything, so a guard placed after it would leave exactly that path throwing NRE — the case the finding is about. First-statement placement also means a null delegate can never take the gate on its way to failing (a later guard would release via `finally`, but only after briefly contending every other caller). Two of the four new tests exist specifically to pin that ordering. +- `RunAsync` is `async`, so its `ArgumentNullException` surfaces on the returned task rather than at the call. Framework convention for async argument validation, and what all four device call sites observe anyway; noted in the XML docs rather than worked around. +- Tests: +4 (null on the normal path and the re-entrant path, sync and async, each also asserting the gate is still free afterward). **Mutation-verified** — deleting the two guards fails exactly those 4 and nothing else (25 → 21 passing in `LifecycleGateTests`). FULL suite green net9 + net10 (**2,592 passed, 2 skipped each**; was 2,588) + `Daqifi.Mcp.Tests` 23. Release solution build **0 warnings** both TFMs. +- No bench re-run, and the reason is specific rather than a shrug: all four device call sites pass non-null lambdas, so the guard is unreachable from the device and cannot alter the connect/disconnect behavior already validated on the real Nq1 for this branch. A bench run could not have observed the change. +- Process note: the log entry was pushed **before** re-requesting review, so `/agentic_review` pins the final SHA rather than a stale one. Prior fires appended the log inside the PR's own commit; a shepherd fire has no PR of its own, so it appends to the branch it shepherded. +- Result: `ff2e30b` pushed to `refactor/344-lifecycle-gate`, thread replied to and resolved, `/agentic_review` re-run. Not merging. Still **3 loop PRs awaiting review (#434, #436, #437) — at the cap**; the next fire shepherds only until the user merges one. + +## 2026-08-05 — Fire: ready-note on #437 + CLI-level bench re-validation of the lifecycle gate +- State at start (re-derived from `gh`, not this log): 3 open loop PRs — **#434, #436, #437 — at the concurrency cap**, so priority 4 was out of scope by rule and this fire was shepherd-only. Backlog unchanged. +- Priority 1: GraphQL `reviewThreads` shows **0 unresolved threads on all three**. #437's single Qodo finding (the `LifecycleGate` null-guard) is resolved, and Qodo re-reviewed the fix commit automatically at 22:56 — `Bugs (0) / Rule violations (0) / Skill insights (0)` against `22d060b`, the current head. Priority 3: CI `build` green on every head SHA. +- Priority 2: **#437 was Qodo-clean + CI green with no ready-note** (the previous fire pushed `ff2e30b` and re-requested review, then ended before the result landed) → posted one. #434 and #436 already had theirs. +- At the cap with nothing else actionable, spent the fire on the saturation clause: **one light bench pass on a surface the previous fire's bench did not cover.** That fire drove `LifecycleGate` through a scratch API-level harness; this one drives the same branch through the **shipped example CLI**, i.e. the ordinary consumer path a user actually takes, with the CLI built against this worktree's core (`-p:DaqifiCoreProjectPath=…`, 0 warnings). +- BENCH (real Nq1 on the bench USB CDC port, fw 3.7.2, non-destructive — connect/stream/disconnect only; no NVM write, no reboot, no SD): 5 full process-level connect → populate → stream → stop → disconnect cycles, **all exit 0**, each reporting `analogIn=16 digital=16 fw=3.7.2` plus a stable serial number (a populated device, not a bare connect). 3 s @ 10 Hz ch 0+1 gave 22 frames, then 4× 1 s @ 20 Hz gave 14 each — consistent run to run, no drift, no wedge across repeated open/close of the same port. +- Also exercised the **failure path the gate has to release on**: a connect to a nonexistent port fails cleanly with the typed `SerialPortConnectException` ("was not found", the #427 naming) and exit 1, and the very next real connect on the same port streams normally. A gate that leaked its permit on the throwing path would strand the following run. +- Bench-rig note: the example CLI's `--duration 0` means **run until Ctrl+C**, not "don't stream" — it is not a way to ask for a connect-only run, and it will hang an unattended fire. Always pass a positive `--duration`. Also, macOS has neither `timeout` nor `gtimeout` on this box; bound runs with the tool timeout and redirect to a file (piping to `tail` buffers all output until the process exits, which hides a hang). +- Pushing the log entry drew a Qodo finding of its own ("Hardware identifiers committed", `SESSION_LOG.md`): the bench note pinned the unit's **serial number** into a public repo. **Valid and taken** — the serial carried no analytical weight (the point was "the device came back populated", not *which* device), so it is now described rather than quoted. Kept the `/dev/cu.usbmodem*` shape, which `README.md` already documents as the ordinary macOS port example and is not an identifier. The same redaction was applied to the PR comment. **Convention for future fires: bench notes state what the device reported, never its serial.** +- CI flake, NOT a regression: the docs-only push failed `DaqifiDeviceOperationSerializationTests.TextExchange_CancelledWhileTheOutboundQueueDrains_DoesNotResubscribeTheConsumer` on **net10 only** — a commit that touched one markdown file cannot regress it. The prior push (`ff2e30b`) failed a *different* net10 timing test (`StreamMessageConsumerStallingReaderTests.Start_WhenStoppedReaderExitsWithinGrace_WaitsAndRestartsSameInstance`). Two different timing tests failing on the same TFM across two runs is runner slowness, not one broken test; both pass locally on net9 + net10. Worth watching: **net10 CI appears to be the flaky lane on this repo**, and if it keeps costing reruns it deserves its own issue rather than a rerun each time. +- Result: no source change this fire. #437 ready-note + bench comment posted; log appended to the branch it shepherded, per the convention set by the previous shepherd fire. Still **3 loop PRs awaiting review (#434, #436, #437) — at the cap**; the next fire shepherds only until the user merges one. diff --git a/src/Daqifi.Core.Tests/Device/Internal/LifecycleGateTests.cs b/src/Daqifi.Core.Tests/Device/Internal/LifecycleGateTests.cs new file mode 100644 index 0000000..572f4e7 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/Internal/LifecycleGateTests.cs @@ -0,0 +1,527 @@ +using Daqifi.Core.Device.Internal; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using System; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Daqifi.Core.Tests.Device.Internal; + +/// +/// Unit tests for , the connect-against-disconnect serialization +/// extracted from DaqifiDevice (issue #379). +/// +/// +/// The gate's behaviour through the device is already covered by DeviceReconnectTests, +/// which races a real reconnect loop against a caller's connect over a scripted transport. These +/// exercise the collaborator directly, which is what the extraction newly makes possible: no +/// device, no transport, no reconnect loop, so contention is produced deliberately rather than +/// raced into, and the policies that were previously only observable through their side effects on +/// a device can be asserted on their own. +/// +public class LifecycleGateTests +{ + /// How long a test waits for something that should already have happened. + private static readonly TimeSpan EventTimeout = TimeSpan.FromSeconds(5); + + private static LifecycleGate Create( + ILogger? logger = null, + string name = "Nq1", + TimeSpan? connectTimeout = null, + TimeSpan? teardownTimeout = null) + => new( + logger ?? NullLogger.Instance, + () => name, + () => connectTimeout ?? TimeSpan.FromMilliseconds(100), + () => teardownTimeout ?? TimeSpan.FromMilliseconds(150)); + + /// + /// Parks an operation inside the gate's critical section and returns a handle that releases it. + /// Every contention test needs a holder; racing two real operations would make the tests + /// depend on scheduling, which is the thing they exist to be independent of. + /// + private static (ManualResetEventSlim Release, Task Holder) HoldGate(LifecycleGate gate) + { + var entered = new ManualResetEventSlim(false); + var release = new ManualResetEventSlim(false); + + var holder = Task.Run(() => gate.Run( + () => + { + entered.Set(); + release.Wait(EventTimeout); + }, + LifecycleContention.Abandon)); + + Assert.True(entered.Wait(EventTimeout), "the holder never entered the gate"); + return (release, holder); + } + + #region Construction + + [Fact] + public void Constructor_WithNullLogger_Throws() + { + Assert.Throws(() => new LifecycleGate( + null!, () => "Nq1", () => TimeSpan.Zero, () => TimeSpan.Zero)); + } + + [Fact] + public void Constructor_WithNullDeviceName_Throws() + { + Assert.Throws(() => new LifecycleGate( + NullLogger.Instance, null!, () => TimeSpan.Zero, () => TimeSpan.Zero)); + } + + [Fact] + public void Constructor_WithNullConnectTimeout_Throws() + { + Assert.Throws(() => new LifecycleGate( + NullLogger.Instance, () => "Nq1", null!, () => TimeSpan.Zero)); + } + + [Fact] + public void Constructor_WithNullTeardownTimeout_Throws() + { + Assert.Throws(() => new LifecycleGate( + NullLogger.Instance, () => "Nq1", () => TimeSpan.Zero, null!)); + } + + [Fact] + public void Run_WithNullOperation_ThrowsArgumentNullExceptionAndLeavesTheGateFree() + { + var gate = Create(); + + Assert.Throws(() => gate.Run(null!, LifecycleContention.Fail)); + + // The guard has to run before the gate is taken, not just before the invocation: a null + // delegate that acquired on its way to failing would leave the next caller contended. + Assert.True(gate.Run(() => { }, LifecycleContention.Fail)); + } + + [Fact] + public async Task RunAsync_WithNullOperation_ThrowsArgumentNullExceptionAndLeavesTheGateFree() + { + var gate = Create(); + + await Assert.ThrowsAsync(() => gate.RunAsync( + null!, LifecycleContention.Fail, CancellationToken.None)); + + Assert.True(await gate.RunAsync( + () => Task.CompletedTask, LifecycleContention.Fail, CancellationToken.None)); + } + + [Fact] + public void Run_WithNullOperationOnTheReentrantPath_StillThrowsArgumentNullException() + { + // The re-entry branch invokes the delegate without acquiring anything, so a guard placed + // after it would leave exactly this path throwing NullReferenceException instead. + var gate = Create(); + Exception? inner = null; + + gate.Run( + () => inner = Record.Exception(() => gate.Run(null!, LifecycleContention.Abandon)), + LifecycleContention.Fail); + + Assert.IsType(inner); + } + + [Fact] + public async Task RunAsync_WithNullOperationOnTheReentrantPath_StillThrowsArgumentNullException() + { + var gate = Create(); + Exception? inner = null; + + await gate.RunAsync( + async () => inner = await Record.ExceptionAsync(() => gate.RunAsync( + null!, LifecycleContention.Abandon, CancellationToken.None)), + LifecycleContention.Fail, + CancellationToken.None); + + Assert.IsType(inner); + } + + #endregion + + #region Uncontended + + [Fact] + public void Run_WhenUncontended_RunsTheOperationAndReportsThatItRan() + { + var ran = false; + + var didRun = Create().Run(() => ran = true, LifecycleContention.Fail); + + Assert.True(didRun); + Assert.True(ran); + } + + [Fact] + public async Task RunAsync_WhenUncontended_RunsTheOperationAndReportsThatItRan() + { + var ran = false; + + var didRun = await Create().RunAsync( + () => { ran = true; return Task.CompletedTask; }, + LifecycleContention.Fail, + CancellationToken.None); + + Assert.True(didRun); + Assert.True(ran); + } + + [Fact] + public void Run_AfterAPreviousOperationCompleted_CanAcquireAgain() + { + // The gate has one permit, so a release that did not happen would show up as the second + // call blocking until its timeout rather than running. + var gate = Create(); + gate.Run(() => { }, LifecycleContention.Fail); + + var didRun = gate.Run(() => { }, LifecycleContention.Fail); + + Assert.True(didRun); + } + + #endregion + + #region Contention policy + + [Fact] + public async Task Run_WhenContendedWithFail_ThrowsWithoutRunningTheOperation() + { + // Running alongside is what the gate exists to prevent: two threads would both find no + // message consumer, both start one, and leave two readers on one stream. + var gate = Create(name: "Contended Nq1"); + var (release, holder) = HoldGate(gate); + var ran = false; + + try + { + var thrown = Assert.Throws( + () => gate.Run(() => ran = true, LifecycleContention.Fail)); + + Assert.Contains("Contended Nq1", thrown.Message, StringComparison.Ordinal); + Assert.Contains("Nothing was opened", thrown.Message, StringComparison.OrdinalIgnoreCase); + Assert.False(ran, "the operation ran alongside the holder"); + } + finally + { + release.Set(); + await holder; + } + } + + [Fact] + public async Task RunAsync_WhenContendedWithFail_ThrowsWithoutRunningTheOperation() + { + var gate = Create(); + var (release, holder) = HoldGate(gate); + var ran = false; + + try + { + await Assert.ThrowsAsync(() => gate.RunAsync( + () => { ran = true; return Task.CompletedTask; }, + LifecycleContention.Fail, + CancellationToken.None)); + + Assert.False(ran); + } + finally + { + release.Set(); + await holder; + } + } + + [Fact] + public async Task Run_WhenContendedWithAbandon_ReportsItDidNotRunRatherThanThrowing() + { + // Teardown is the resource-release path and Dispose depends on it, so contention must not + // turn it into an exception — and it must not run alongside either. + var gate = Create(); + var (release, holder) = HoldGate(gate); + var ran = false; + + try + { + var didRun = gate.Run(() => ran = true, LifecycleContention.Abandon); + + Assert.False(didRun); + Assert.False(ran); + } + finally + { + release.Set(); + await holder; + } + } + + [Fact] + public async Task RunAsync_WhenContendedWithAbandon_ReportsItDidNotRunRatherThanThrowing() + { + var gate = Create(); + var (release, holder) = HoldGate(gate); + var ran = false; + + try + { + var didRun = await gate.RunAsync( + () => { ran = true; return Task.CompletedTask; }, + LifecycleContention.Abandon, + CancellationToken.None); + + Assert.False(didRun); + Assert.False(ran); + } + finally + { + release.Set(); + await holder; + } + } + + [Fact] + public void ContentionWait_UsesTheTeardownBudgetForAbandonAndTheConnectBudgetForFail() + { + // A shared budget would suit neither caller: a connect that waits the teardown budget + // stalls, and a teardown that waits the connect budget gives up on a holder that was + // about to finish. + var connectReads = 0; + var teardownReads = 0; + var gate = new LifecycleGate( + NullLogger.Instance, + () => "Nq1", + () => { connectReads++; return TimeSpan.FromMilliseconds(50); }, + () => { teardownReads++; return TimeSpan.FromMilliseconds(50); }); + + gate.Run(() => { }, LifecycleContention.Fail); + Assert.Equal(1, connectReads); + Assert.Equal(0, teardownReads); + + gate.Run(() => { }, LifecycleContention.Abandon); + Assert.Equal(1, connectReads); + Assert.Equal(1, teardownReads); + } + + [Fact] + public async Task ContentionWait_ReadsTheTimeoutWhenContentionHappens_NotAtConstruction() + { + // This is why the timeouts arrive as delegates rather than values. They are virtual on the + // device so a test can shorten them, and a test subclass assigns its override through an + // init property that runs AFTER the base constructor builds the gate — so a gate that + // captured them at construction would silently ignore every override. + var connectTimeout = TimeSpan.FromMinutes(10); + var gate = new LifecycleGate( + NullLogger.Instance, + () => "Nq1", + () => connectTimeout, + () => TimeSpan.FromMinutes(10)); + + // Shortened after construction, exactly as an init-set override is. + connectTimeout = TimeSpan.FromMilliseconds(50); + + var (release, holder) = HoldGate(gate); + try + { + // Would block for ten minutes if the construction-time value were the one in force. + var contended = Task.Run(() => gate.Run(() => { }, LifecycleContention.Fail)); + + Assert.Same( + contended, + await Task.WhenAny(contended, Task.Delay(EventTimeout))); + await Assert.ThrowsAsync(() => contended); + } + finally + { + release.Set(); + await holder; + } + } + + #endregion + + #region Re-entry + + [Fact] + public void Run_WhenReenteredFromInsideTheCriticalSection_ProceedsInsteadOfDeadlocking() + { + // Both connect and disconnect raise StatusChanged from inside their critical section, and + // a consumer handler calling Disconnect from there is re-entry on the same flow. It ran + // nested before the gate existed and has to keep working against a non-reentrant semaphore. + var gate = Create(); + var innerRan = false; + + var didRun = gate.Run( + () => Assert.True(gate.Run(() => innerRan = true, LifecycleContention.Abandon)), + LifecycleContention.Fail); + + Assert.True(didRun); + Assert.True(innerRan); + } + + [Fact] + public async Task RunAsync_WhenReenteredAcrossAnAwait_ProceedsInsteadOfDeadlocking() + { + // AsyncLocal rather than a thread id precisely so the re-entry flag survives a + // continuation resuming on a different thread. + var gate = Create(); + var innerRan = false; + + var didRun = await gate.RunAsync( + async () => + { + await Task.Yield(); + Assert.True(await gate.RunAsync( + () => { innerRan = true; return Task.CompletedTask; }, + LifecycleContention.Abandon, + CancellationToken.None)); + }, + LifecycleContention.Fail, + CancellationToken.None); + + Assert.True(didRun); + Assert.True(innerRan); + } + + [Fact] + public async Task Run_AfterAReentrantOperationCompletes_TreatsTheNextCallAsContendedAgain() + { + // The re-entry flag has to be cleared on the way out. If it leaked, a later caller on a + // flow that had once been inside the gate would sail past a holder — the double-open the + // gate exists to prevent, and invisible to a test that only checks the nested call works. + var gate = Create(); + gate.Run(() => gate.Run(() => { }, LifecycleContention.Abandon), LifecycleContention.Fail); + + var (release, holder) = HoldGate(gate); + var ran = false; + try + { + Assert.False(gate.Run(() => ran = true, LifecycleContention.Abandon)); + Assert.False(ran); + } + finally + { + release.Set(); + await holder; + } + } + + #endregion + + #region Cancellation + + [Fact] + public async Task RunAsync_ForTeardownWithAnAlreadyCancelledToken_StillRunsTheTeardown() + { + // Regression guard for the #341 contract. SemaphoreSlim.WaitAsync throws for an + // already-cancelled token even when the semaphore is free, so passing a teardown's token + // to the acquire made EVERY cancelled disconnect skip teardown — reporting Disconnected + // with the transport still open and the message pumps still running — not just a contended + // one. The token means "shorten the courtesy wait for an in-flight exchange", never + // "abandon the disconnect". + using var cts = new CancellationTokenSource(); + cts.Cancel(); + var ran = false; + + var didRun = await Create().RunAsync( + () => { ran = true; return Task.CompletedTask; }, + LifecycleContention.Abandon, + cts.Token); + + Assert.True(didRun); + Assert.True(ran); + } + + [Fact] + public async Task RunAsync_ForConnectWithAnAlreadyCancelledToken_HonoursTheToken() + { + // The connect path is the opposite case: ConnectAsync is documented to be abandonable and + // to surface an OperationCanceledException. + using var cts = new CancellationTokenSource(); + cts.Cancel(); + var ran = false; + + await Assert.ThrowsAnyAsync(() => Create().RunAsync( + () => { ran = true; return Task.CompletedTask; }, + LifecycleContention.Fail, + cts.Token)); + + Assert.False(ran); + } + + #endregion + + #region Release + + [Fact] + public void Run_WhenTheOperationThrows_StillReleasesTheGate() + { + // A connect that fails must not leave the gate held: every later connect and — worse — + // every later teardown would then be permanently contended. + var gate = Create(); + + Assert.Throws(() => gate.Run( + () => throw new InvalidOperationException("connect-boom"), + LifecycleContention.Fail)); + + Assert.True(gate.Run(() => { }, LifecycleContention.Fail)); + } + + [Fact] + public async Task RunAsync_WhenTheOperationThrows_StillReleasesTheGate() + { + var gate = Create(); + + await Assert.ThrowsAsync(() => gate.RunAsync( + () => throw new InvalidOperationException("connect-boom"), + LifecycleContention.Fail, + CancellationToken.None)); + + Assert.True(await gate.RunAsync( + () => Task.CompletedTask, LifecycleContention.Fail, CancellationToken.None)); + } + + #endregion + + #region Logger isolation + + [Fact] + public async Task Run_WhenContendedAndTheLoggerThrows_StillReportsTheContention() + { + // A consumer-supplied logger must never affect device operation, least of all on the path + // that is already reporting trouble. + var gate = Create(logger: new ThrowingLogger()); + var (release, holder) = HoldGate(gate); + + try + { + Assert.Throws(() => gate.Run(() => { }, LifecycleContention.Fail)); + Assert.False(gate.Run(() => { }, LifecycleContention.Abandon)); + } + finally + { + release.Set(); + await holder; + } + } + + private sealed class ThrowingLogger : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + => throw new InvalidOperationException("logger-boom"); + } + + #endregion +} diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index dc3f334..75b2c44 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -605,6 +605,11 @@ public DaqifiDevice(string name, IPAddress? ipAddress = null, ILogger? logger = _status = ConnectionStatus.Disconnected; _logger = logger ?? NullLogger.Instance; _channelPopulator = new StatusChannelPopulator(_logger, () => Name); + _lifecycleGate = new LifecycleGate( + _logger, + () => Name, + () => LifecycleLockTimeout, + () => TeardownLockTimeout); } /// @@ -621,6 +626,11 @@ public DaqifiDevice(string name, Stream stream, IPAddress? ipAddress = null, ILo _status = ConnectionStatus.Disconnected; _logger = logger ?? NullLogger.Instance; _channelPopulator = new StatusChannelPopulator(_logger, () => Name); + _lifecycleGate = new LifecycleGate( + _logger, + () => Name, + () => LifecycleLockTimeout, + () => TeardownLockTimeout); _messageProducer = new MessageProducer(stream); _messageProducer.SendFailed += OnMessageSendFailed; _directStream = stream; @@ -638,6 +648,11 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n _status = ConnectionStatus.Disconnected; _logger = logger ?? NullLogger.Instance; _channelPopulator = new StatusChannelPopulator(_logger, () => Name); + _lifecycleGate = new LifecycleGate( + _logger, + () => Name, + () => LifecycleLockTimeout, + () => TeardownLockTimeout); _transport = transport; // Subscribe to transport status changes @@ -648,46 +663,17 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n /// /// Serializes connect against disconnect, on both the synchronous and the asynchronous - /// paths. + /// paths, so the device never drives its transport from two threads at once. See + /// for why that invariant exists and what each contention + /// policy does. /// /// - /// - /// Automatic reconnection (issue #379) introduced a second thread that opens and closes the - /// transport, and cancellation is not synchronization: - /// asks the loop to stop and returns immediately, but a loop already inside a blocking - /// Connect() cannot be interrupted and will run to completion. Without this, a - /// caller's could be opening and closing the same serial port - /// concurrently, and both threads could build and start a message consumer — leaving two - /// readers on one stream, the framing corruption this class refuses to risk anywhere else. - /// - /// - /// Narrow on purpose. This is an internal lifecycle invariant — the device never drives its - /// own transport from two threads at once — and deliberately not the general - /// per-device operation serialization of issue #342, which has to decide ordering across - /// the whole public API and interacts with _textExchangeLock. Nothing here changes - /// what any public method does when uncontended. - /// - /// - /// A semaphore rather than a monitor because and - /// hold it across await, which a monitor cannot do — - /// its continuation may resume on a different thread. Semaphores are not reentrant, so - /// re-entry is tracked separately by . - /// - /// - private readonly SemaphoreSlim _lifecycleLock = new(1, 1); - - /// - /// True while the current logical flow already holds . - /// - /// - /// Both connect and disconnect raise from inside their critical - /// section, and a consumer handler calling from there is re-entry - /// on the same flow — which runs nested today with no lock at all and must keep working - /// rather than deadlock against a non-reentrant semaphore. - /// rather than a thread id so it survives an await resuming on another thread, the - /// same technique _isInsideTextExchange already uses in this class. + /// The timeouts are handed over as delegates so the gate reads them at the moment of + /// contention: they are virtual below precisely so a test can shorten them, and a + /// test subclass sets its override through an init property that runs after this + /// constructor. Reading them here would capture the defaults and ignore every override. /// - private readonly AsyncLocal _isInsideLifecycleOperation = new(); + private readonly LifecycleGate _lifecycleGate; /// /// How long waits for a lifecycle operation already in flight before @@ -703,224 +689,6 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n /// internal virtual TimeSpan TeardownLockTimeout => TimeSpan.FromSeconds(30); - /// - /// What a lifecycle operation does when it cannot have to - /// itself. Running anyway is deliberately not an option: the whole point of the lock is - /// that two threads must never drive the transport at once, and a guarantee with a - /// "proceed regardless" branch is not a guarantee. - /// - private enum LifecycleContention - { - /// - /// Give up and throw rather than run alongside. For : nothing has - /// been opened, so failing costs the caller a retry and nothing else. - /// - Fail, - - /// - /// Give up and report it, leaving the operation in flight alone. For - /// : the caller is told nothing was torn down rather than being - /// blocked forever behind a holder that may never return. - /// - Abandon - } - - /// - /// The wait a contention policy allows, and what to do when it runs out. - /// - /// - /// - /// The two callers want opposite things from contention, so neither a shared timeout nor a - /// shared fallback would suit both. - /// - /// - /// fails. It waits and then - /// throws. Opening a second connection alongside one already in flight is exactly what this - /// lock exists to prevent — both threads would find no message consumer, both would build - /// and start one, and the loser's reader would be left running on the same stream, silently - /// corrupting frame boundaries for the rest of the session. A caller who gets a - /// instead has lost nothing: no handle was opened, no state - /// changed, and they can try again. - /// - /// - /// abandons. It waits — - /// far longer, because a teardown that gives up early is a teardown that did not happen — - /// and then reports that it did not run, leaving the holder alone. It must not throw - /// (Dispose depends on it) and must not run alongside (that is the corruption - /// above). An unbounded wait is not an option either: SerialPort.Open is called - /// synchronously with no timeout and can wedge in uncancellable native I/O — a hazard this - /// codebase already knows well enough to have built a process-wide port quarantine around - /// it in — so waiting on it forever would turn - /// Dispose into a permanent block. Abandoning the stuck operation is the house - /// answer to uncancellable native I/O here. - /// - /// - private TimeSpan ContentionWait(LifecycleContention onContention) => - onContention == LifecycleContention.Abandon ? TeardownLockTimeout : LifecycleLockTimeout; - - /// - /// Builds the failure for a connect that could not have the lock to itself. - /// - private TimeoutException LifecycleTimeout(TimeSpan timeout) - { - SafeLog(() => _logger.LogError( - "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " - + "within {TimeoutSeconds}s; refusing to connect alongside the operation in flight.", - Name, - timeout.TotalSeconds)); - - return new TimeoutException( - $"Device '{Name}' could not start connecting within " - + $"{timeout.TotalSeconds:0.#}s because another connect or disconnect " - + "was still in progress. Nothing was opened; retry once it has finished."); - } - - /// Reports a teardown that gave up waiting for a stuck lifecycle operation. - private void LogAbandonedTeardown(TimeSpan timeout) => - SafeLog(() => _logger.LogError( - "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " - + "within {TimeoutSeconds}s, so nothing was torn down. A connect is most likely " - + "wedged in uncancellable native I/O; it will release its own session when it " - + "returns.", - Name, - timeout.TotalSeconds)); - - /// - /// Runs a lifecycle operation under , never alongside another. - /// See for the per-policy semantics. - /// - /// true if the operation ran; false if the wait was abandoned. - /// - /// Thrown when is and - /// another lifecycle operation held the lock for the whole timeout. - /// - private bool RunLifecycleExclusive(Action operation, LifecycleContention onContention) - { - // Re-entry from inside the critical section (a StatusChanged handler calling back in) - // proceeds without acquiring, exactly as a reentrant monitor would. - if (_isInsideLifecycleOperation.Value) - { - operation(); - return true; - } - - var timeout = ContentionWait(onContention); - var acquired = false; - - try - { - acquired = _lifecycleLock.Wait(timeout); - } - catch (ObjectDisposedException) - { - // Disposed underneath us; there is nothing left to serialize against. - operation(); - return true; - } - - if (!acquired) - { - if (onContention == LifecycleContention.Abandon) - { - LogAbandonedTeardown(timeout); - return false; - } - - throw LifecycleTimeout(timeout); - } - - _isInsideLifecycleOperation.Value = true; - try - { - operation(); - return true; - } - finally - { - _isInsideLifecycleOperation.Value = false; - ReleaseLifecycleLock(); - } - } - - /// - private async Task RunLifecycleExclusiveAsync( - Func operation, - LifecycleContention onContention, - CancellationToken cancellationToken) - { - if (_isInsideLifecycleOperation.Value) - { - await operation().ConfigureAwait(false); - return true; - } - - var timeout = ContentionWait(onContention); - var isTeardown = onContention == LifecycleContention.Abandon; - var acquired = false; - - // A teardown's token is NOT allowed to govern this wait. Issue #341 defines what the - // token means for DisconnectAsync — it shortens the courtesy wait for an in-flight - // command exchange, never aborts the disconnect, and never surfaces as an - // OperationCanceledException — and this lock is a second, later wait that contract - // never covered. Passing the token here made a cancelled DisconnectAsync skip teardown - // altogether and report Disconnected with the transport still open and the message - // pumps still running; and because SemaphoreSlim throws for an already-cancelled token - // even when the semaphore is free, that happened on every cancelled disconnect, not - // just a contended one. The wait stays bounded by TeardownLockTimeout, which is what - // protects against a genuinely wedged holder (issue #379). The token still reaches - // AcquireTextExchangeLockForTeardownAsync inside the teardown, where it means what - // #341 says it means. - // - // The connect path is the opposite case and does honour the token: ConnectAsync is - // documented to be abandonable and to throw OperationCanceledException. - var acquireToken = isTeardown ? CancellationToken.None : cancellationToken; - - try - { - acquired = await _lifecycleLock.WaitAsync(timeout, acquireToken).ConfigureAwait(false); - } - catch (ObjectDisposedException) - { - await operation().ConfigureAwait(false); - return true; - } - - if (!acquired) - { - if (isTeardown) - { - LogAbandonedTeardown(timeout); - return false; - } - - throw LifecycleTimeout(timeout); - } - - _isInsideLifecycleOperation.Value = true; - try - { - await operation().ConfigureAwait(false); - return true; - } - finally - { - _isInsideLifecycleOperation.Value = false; - ReleaseLifecycleLock(); - } - } - - private void ReleaseLifecycleLock() - { - try - { - _lifecycleLock.Release(); - } - catch (ObjectDisposedException) - { - // Raced a Dispose that already tore the semaphore down. - } - } - #endregion #region Operation serialization (issue #342) @@ -932,8 +700,8 @@ private void ReleaseLifecycleLock() /// /// Set by and by the text exchange. /// rather than a thread id so it survives an await - /// resuming on another thread — the same technique _isInsideLifecycleOperation and - /// _isInsideTextExchange use. + /// resuming on another thread — the same technique _isInsideTextExchange and + /// 's re-entry flag use. /// /// A generation rather than a bool because holding the lock and owning the current /// session are different questions, and teardown separates them. See @@ -1443,14 +1211,14 @@ public void Connect() /// private void ConnectCore() { - RunLifecycleExclusive(ConnectCoreUnsynchronized, LifecycleContention.Fail); + _lifecycleGate.Run(ConnectCoreUnsynchronized, LifecycleContention.Fail); HonourTeardownRaisedDuringConnect(); } /// private async Task ConnectCoreAsync(CancellationToken cancellationToken) { - await RunLifecycleExclusiveAsync( + await _lifecycleGate.RunAsync( () => ConnectCoreUnsynchronizedAsync(cancellationToken), LifecycleContention.Fail, cancellationToken).ConfigureAwait(false); @@ -1735,7 +1503,7 @@ public void Disconnect() /// private void DisconnectCore(ConnectionStatus finalStatus) { - if (RunLifecycleExclusive( + if (_lifecycleGate.Run( () => DisconnectCoreUnsynchronized(finalStatus), LifecycleContention.Abandon)) { @@ -1748,7 +1516,7 @@ private void DisconnectCore(ConnectionStatus finalStatus) /// private async Task DisconnectCoreAsync(ConnectionStatus finalStatus, CancellationToken cancellationToken) { - if (await RunLifecycleExclusiveAsync( + if (await _lifecycleGate.RunAsync( () => DisconnectCoreUnsynchronizedAsync(finalStatus, cancellationToken), LifecycleContention.Abandon, cancellationToken).ConfigureAwait(false)) diff --git a/src/Daqifi.Core/Device/Internal/LifecycleGate.cs b/src/Daqifi.Core/Device/Internal/LifecycleGate.cs new file mode 100644 index 0000000..bd149b9 --- /dev/null +++ b/src/Daqifi.Core/Device/Internal/LifecycleGate.cs @@ -0,0 +1,359 @@ +using Microsoft.Extensions.Logging; +using System; +using System.Threading; +using System.Threading.Tasks; + +#nullable enable + +namespace Daqifi.Core.Device.Internal +{ + /// + /// What a lifecycle operation does when it cannot have the gate to itself. Running anyway is + /// deliberately not an option: the whole point of the gate is that two threads must never drive + /// the transport at once, and a guarantee with a "proceed regardless" branch is not a guarantee. + /// + internal enum LifecycleContention + { + /// + /// Give up and throw rather than run alongside. For : + /// nothing has been opened, so failing costs the caller a retry and nothing else. + /// + Fail, + + /// + /// Give up and report it, leaving the operation in flight alone. For + /// : the caller is told nothing was torn down rather + /// than being blocked forever behind a holder that may never return. + /// + Abandon + } + + /// + /// Serializes connect against disconnect, on both the synchronous and the asynchronous paths, + /// extracted from so the device delegates rather than hosts it + /// (issue #379). + /// + /// + /// + /// Automatic reconnection (issue #379) introduced a second thread that opens and closes the + /// transport, and cancellation is not synchronization: SupersedeReconnect asks the loop + /// to stop and returns immediately, but a loop already inside a blocking Connect() + /// cannot be interrupted and will run to completion. Without this, a caller's + /// could be opening and closing the same serial port + /// concurrently, and both threads could build and start a message consumer — leaving two + /// readers on one stream, the framing corruption the device refuses to risk anywhere else. + /// + /// + /// Narrow on purpose. This is an internal lifecycle invariant — the device never drives its own + /// transport from two threads at once — and deliberately not the general per-device + /// operation serialization of issue #342, which has to decide ordering across the whole public + /// API and interacts with _textExchangeLock. Nothing here changes what any public method + /// does when uncontended. + /// + /// + /// A semaphore rather than a monitor because and + /// hold it across await, which a monitor + /// cannot do — its continuation may resume on a different thread. Semaphores are not reentrant, + /// so re-entry is tracked separately by . + /// + /// + /// The semaphore is deliberately never disposed, matching the device's own resource teardown, + /// which has never disposed it. The handlers below are + /// therefore defensive rather than reachable today; they are kept so a future teardown that + /// does dispose it degrades to "nothing left to serialize against" instead of throwing out of + /// a disconnect. + /// + /// + internal sealed class LifecycleGate + { + private readonly ILogger _logger; + private readonly Func _deviceName; + private readonly Func _connectTimeout; + private readonly Func _teardownTimeout; + + /// + /// The gate itself. One permit: a lifecycle operation either has it or waits for it. + /// + private readonly SemaphoreSlim _lifecycleLock = new(1, 1); + + /// + /// True while the current logical flow already holds . + /// + /// + /// Both connect and disconnect raise from inside + /// their critical section, and a consumer handler calling + /// from there is re-entry on the same flow — which + /// runs nested today with no lock at all and must keep working rather than deadlock against + /// a non-reentrant semaphore. rather than a thread id so it + /// survives an await resuming on another thread, the same technique + /// _isInsideTextExchange already uses on the device. + /// + private readonly AsyncLocal _isInsideLifecycleOperation = new(); + + /// + /// Creates a gate that logs against the owning device and reads its contention timeouts. + /// + /// The device's logger; contention reports go here. + /// + /// Reads the owning device's current name for those reports. A delegate rather than a + /// captured string because the name can change during the device's lifetime, and a report + /// naming the wrong device is worse than one naming none. + /// + /// + /// Reads at the moment of contention. + /// + /// + /// Reads at the moment of contention. + /// + /// + /// The two timeouts arrive as delegates, not values, and that is load-bearing rather than + /// stylistic. They are virtual on the device precisely so a test can shorten them, + /// and a test subclass sets its override through an init property — which runs + /// after the base constructor that builds this gate. Reading them here would capture + /// the base defaults (10 s / 30 s) and silently ignore every override. + /// + internal LifecycleGate( + ILogger logger, + Func deviceName, + Func connectTimeout, + Func teardownTimeout) + { + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _deviceName = deviceName ?? throw new ArgumentNullException(nameof(deviceName)); + _connectTimeout = connectTimeout ?? throw new ArgumentNullException(nameof(connectTimeout)); + _teardownTimeout = teardownTimeout ?? throw new ArgumentNullException(nameof(teardownTimeout)); + } + + /// + /// The wait a contention policy allows, and what to do when it runs out. + /// + /// + /// + /// The two callers want opposite things from contention, so neither a shared timeout nor a + /// shared fallback would suit both. + /// + /// + /// fails. It waits + /// and then throws. Opening a second + /// connection alongside one already in flight is exactly what this gate exists to prevent — + /// both threads would find no message consumer, both would build and start one, and the + /// loser's reader would be left running on the same stream, silently corrupting frame + /// boundaries for the rest of the session. A caller who gets a + /// instead has lost nothing: no handle was opened, no state + /// changed, and they can try again. + /// + /// + /// abandons. It waits + /// — far longer, because a teardown that + /// gives up early is a teardown that did not happen — and then reports that it did not run, + /// leaving the holder alone. It must not throw (Dispose depends on it) and must not + /// run alongside (that is the corruption above). An unbounded wait is not an option either: + /// SerialPort.Open is called synchronously with no timeout and can wedge in + /// uncancellable native I/O — a hazard this codebase already knows well enough to have + /// built a process-wide port quarantine around it in + /// — so waiting on it forever + /// would turn Dispose into a permanent block. Abandoning the stuck operation is the + /// house answer to uncancellable native I/O here. + /// + /// + private TimeSpan ContentionWait(LifecycleContention onContention) => + onContention == LifecycleContention.Abandon ? _teardownTimeout() : _connectTimeout(); + + /// + /// Builds the failure for a connect that could not have the gate to itself. + /// + private TimeoutException LifecycleTimeout(TimeSpan timeout) + { + SafeLog(() => _logger.LogError( + "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " + + "within {TimeoutSeconds}s; refusing to connect alongside the operation in flight.", + _deviceName(), + timeout.TotalSeconds)); + + return new TimeoutException( + $"Device '{_deviceName()}' could not start connecting within " + + $"{timeout.TotalSeconds:0.#}s because another connect or disconnect " + + "was still in progress. Nothing was opened; retry once it has finished."); + } + + /// Reports a teardown that gave up waiting for a stuck lifecycle operation. + private void LogAbandonedTeardown(TimeSpan timeout) => + SafeLog(() => _logger.LogError( + "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " + + "within {TimeoutSeconds}s, so nothing was torn down. A connect is most likely " + + "wedged in uncancellable native I/O; it will release its own session when it " + + "returns.", + _deviceName(), + timeout.TotalSeconds)); + + /// + /// Runs a lifecycle operation under , never alongside another. + /// See for the per-policy semantics. + /// + /// true if the operation ran; false if the wait was abandoned. + /// + /// Thrown when is null. + /// + /// + /// Thrown when is and + /// another lifecycle operation held the gate for the whole timeout. + /// + internal bool Run(Action operation, LifecycleContention onContention) + { + // Validated before anything else so a null delegate is reported as misuse rather than + // as a NullReferenceException from one of the three call sites below, and so it can + // never take the gate on its way to failing. Matches the constructor's guards and the + // entry-point guards on the sibling collaborators. + ArgumentNullException.ThrowIfNull(operation); + + // Re-entry from inside the critical section (a StatusChanged handler calling back in) + // proceeds without acquiring, exactly as a reentrant monitor would. + if (_isInsideLifecycleOperation.Value) + { + operation(); + return true; + } + + var timeout = ContentionWait(onContention); + var acquired = false; + + try + { + acquired = _lifecycleLock.Wait(timeout); + } + catch (ObjectDisposedException) + { + // Disposed underneath us; there is nothing left to serialize against. + operation(); + return true; + } + + if (!acquired) + { + if (onContention == LifecycleContention.Abandon) + { + LogAbandonedTeardown(timeout); + return false; + } + + throw LifecycleTimeout(timeout); + } + + _isInsideLifecycleOperation.Value = true; + try + { + operation(); + return true; + } + finally + { + _isInsideLifecycleOperation.Value = false; + ReleaseLifecycleLock(); + } + } + + /// + /// + /// Being an async method, the surfaces on the + /// returned task rather than at the call, which is the framework's own convention for + /// async argument validation and is what every call site here observes anyway. + /// + internal async Task RunAsync( + Func operation, + LifecycleContention onContention, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(operation); + + if (_isInsideLifecycleOperation.Value) + { + await operation().ConfigureAwait(false); + return true; + } + + var timeout = ContentionWait(onContention); + var isTeardown = onContention == LifecycleContention.Abandon; + var acquired = false; + + // A teardown's token is NOT allowed to govern this wait. Issue #341 defines what the + // token means for DisconnectAsync — it shortens the courtesy wait for an in-flight + // command exchange, never aborts the disconnect, and never surfaces as an + // OperationCanceledException — and this gate is a second, later wait that contract + // never covered. Passing the token here made a cancelled DisconnectAsync skip teardown + // altogether and report Disconnected with the transport still open and the message + // pumps still running; and because SemaphoreSlim throws for an already-cancelled token + // even when the semaphore is free, that happened on every cancelled disconnect, not + // just a contended one. The wait stays bounded by TeardownLockTimeout, which is what + // protects against a genuinely wedged holder (issue #379). The token still reaches + // AcquireTextExchangeLockForTeardownAsync inside the teardown, where it means what + // #341 says it means. + // + // The connect path is the opposite case and does honour the token: ConnectAsync is + // documented to be abandonable and to throw OperationCanceledException. + var acquireToken = isTeardown ? CancellationToken.None : cancellationToken; + + try + { + acquired = await _lifecycleLock.WaitAsync(timeout, acquireToken).ConfigureAwait(false); + } + catch (ObjectDisposedException) + { + await operation().ConfigureAwait(false); + return true; + } + + if (!acquired) + { + if (isTeardown) + { + LogAbandonedTeardown(timeout); + return false; + } + + throw LifecycleTimeout(timeout); + } + + _isInsideLifecycleOperation.Value = true; + try + { + await operation().ConfigureAwait(false); + return true; + } + finally + { + _isInsideLifecycleOperation.Value = false; + ReleaseLifecycleLock(); + } + } + + private void ReleaseLifecycleLock() + { + try + { + _lifecycleLock.Release(); + } + catch (ObjectDisposedException) + { + // Raced a Dispose that already tore the semaphore down. + } + } + + /// + /// Runs a logging call, swallowing any exception a misbehaving throws. + /// A consumer-supplied logger must never affect device operation — least of all here, where + /// the only logging happens on a path that is already reporting trouble. Mirrors + /// DaqifiDevice.SafeLog. + /// + private static void SafeLog(Action logAction) + { + try + { + logAction(); + } + catch + { + // A logger that throws is not permitted to take down device operation. + } + } + } +}