From 460a9f1fd06a8bb2ad8948e04e95e6bf01240a0e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 20 Sep 2026 10:30:17 +0000 Subject: [PATCH 1/4] refactor(core): extract shared SafeLog/SafeTrace helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse the copy-pasted try/catch swallows and “mirrors DaqifiDevice.SafeLog” remark walls into one internal DiagnosticGuard. Transport drop-path traces still compose the message inside the guard so a throwing Exception.ToString cannot escape the catch. Co-authored-by: Tyler Kron --- .../Internal/DiagnosticGuardTests.cs | 37 ++++++++++++++++ .../Producers/MessageProducer.cs | 18 +------- .../Transport/SerialStreamTransport.cs | 38 ++-------------- .../Transport/TcpStreamTransport.cs | 25 ++--------- src/Daqifi.Core/Device/DaqifiDevice.cs | 19 +------- .../Device/DaqifiStreamingDevice.cs | 27 +----------- .../Discovery/AllTransportsDeviceFinder.cs | 14 +----- .../Device/Internal/LifecycleGate.cs | 19 +------- .../Device/Internal/OperationSerializer.cs | 17 +------- .../Device/Internal/ReconnectSupervisor.cs | 17 +------- .../Device/Internal/StatusChannelPopulator.cs | 22 +--------- .../Internal/StreamingSessionController.cs | 18 +------- .../Device/Internal/TextExchangeEngine.cs | 17 +------- src/Daqifi.Core/Internal/DiagnosticGuard.cs | 43 +++++++++++++++++++ 14 files changed, 97 insertions(+), 234 deletions(-) create mode 100644 src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs create mode 100644 src/Daqifi.Core/Internal/DiagnosticGuard.cs diff --git a/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs b/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs new file mode 100644 index 00000000..c6ead6ed --- /dev/null +++ b/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs @@ -0,0 +1,37 @@ +using Daqifi.Core.Internal; + +namespace Daqifi.Core.Tests.Internal; + +public class DiagnosticGuardTests +{ + [Fact] + public void SafeLog_RunsTheAction() + { + var ran = false; + DiagnosticGuard.SafeLog(() => ran = true); + Assert.True(ran); + } + + [Fact] + public void SafeLog_SwallowsThrownExceptions() + { + var ex = Record.Exception(() => + DiagnosticGuard.SafeLog(() => throw new InvalidOperationException("boom"))); + Assert.Null(ex); + } + + [Fact] + public void SafeTrace_WritesWithoutThrowing() + { + var ex = Record.Exception(() => DiagnosticGuard.SafeTrace("diagnostic")); + Assert.Null(ex); + } + + [Fact] + public void SafeTrace_SwallowsAThrowingMessageFactory() + { + var ex = Record.Exception(() => + DiagnosticGuard.SafeTrace(static () => throw new InvalidOperationException("cannot render"))); + Assert.Null(ex); + } +} diff --git a/src/Daqifi.Core/Communication/Producers/MessageProducer.cs b/src/Daqifi.Core/Communication/Producers/MessageProducer.cs index 8717f3e6..a71b5247 100644 --- a/src/Daqifi.Core/Communication/Producers/MessageProducer.cs +++ b/src/Daqifi.Core/Communication/Producers/MessageProducer.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using System.Collections.Concurrent; +using static Daqifi.Core.Internal.DiagnosticGuard; namespace Daqifi.Core.Communication.Producers; @@ -340,23 +341,6 @@ private void ProcessMessages() } } - /// - /// Invokes a logging action, swallowing any exception thrown by the logger - /// itself. A faulting logger must never be allowed to terminate the background - /// processing loop or leave the producer in an inconsistent state. - /// - private static void SafeLog(Action logAction) - { - try - { - logAction(); - } - catch - { - // A logger that throws is not permitted to take down the producer. - } - } - /// /// Writes a message to the underlying stream. /// diff --git a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs index 82f60a8f..26b8eb0d 100644 --- a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs +++ b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs @@ -1,4 +1,5 @@ using System.IO.Ports; +using static Daqifi.Core.Internal.DiagnosticGuard; namespace Daqifi.Core.Communication.Transport; @@ -674,7 +675,8 @@ private void HandleConnectionLost(Exception error) // disturbing them. A DaqifiDevice surfaces its own StatusChanged subscriber failures on // ErrorOccurred; this transport carries no logger, so a consumer working against a bare // transport gets the same best-effort trace DeviceFinderBase gives its event raises. - SafeTrace(ex); + SafeTrace(() => + $"[{nameof(SerialStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {ex}"); } finally { @@ -689,40 +691,6 @@ private void HandleConnectionLost(Exception error) } } - /// - /// Writes the diagnostic line for a subscriber failure, swallowing - /// anything a misbehaving throws. - /// - /// - /// dispatches to listeners the consumer installed, so it - /// is consumer code and can throw like any other. A listener throwing out of the catch - /// that was containing a bad subscriber would defeat the containment and cost the port handle - /// anyway. Same guarantee as DeviceFinderBase.RaiseIsolated and - /// DaqifiStreamingDevice.SafeTrace; this transport has no ILogger, hence the local - /// twin rather than a shared one. - /// - /// The message is composed inside the guard rather than passed in ready-made, because - /// came out of consumer code too: rendering an exception - /// whose or throws would otherwise - /// escape the catch from the interpolation itself and leave the drop path exactly as - /// disrupted as an unguarded raise. - /// - /// - /// The exception the subscriber threw. - private static void SafeTrace(Exception subscriberFailure) - { - try - { - System.Diagnostics.Trace.WriteLine( - $"[{nameof(SerialStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {subscriberFailure}"); - } - catch - { - // A trace listener — or an exception that cannot render itself — is not permitted to - // affect the drop path. - } - } - /// /// Raises the StatusChanged event. /// diff --git a/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs b/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs index 0d680c12..2b2a09c0 100644 --- a/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs +++ b/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs @@ -1,5 +1,6 @@ using System.Net; using System.Net.Sockets; +using static Daqifi.Core.Internal.DiagnosticGuard; namespace Daqifi.Core.Communication.Transport; @@ -416,7 +417,8 @@ private void HandleConnectionLost(Exception error) // Disconnect() and Dispose() would both find null and skip them too, leaking the socket // for the life of the process. Not rethrown, and traced best-effort because this // transport carries no logger: see SerialStreamTransport.HandleConnectionLost. - SafeTrace(ex); + SafeTrace(() => + $"[{nameof(TcpStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {ex}"); } finally { @@ -432,27 +434,6 @@ private void HandleConnectionLost(Exception error) } } - /// - /// Writes the diagnostic line for a subscriber failure, swallowing - /// anything a misbehaving throws. See - /// SerialStreamTransport.SafeTrace for why the trace itself — and the composition of its - /// message from a consumer-supplied exception — has to be contained. - /// - /// The exception the subscriber threw. - private static void SafeTrace(Exception subscriberFailure) - { - try - { - System.Diagnostics.Trace.WriteLine( - $"[{nameof(TcpStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {subscriberFailure}"); - } - catch - { - // A trace listener — or an exception that cannot render itself — is not permitted to - // affect the drop path. - } - } - /// /// Raises the StatusChanged event. /// diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 04c919cb..c07028c8 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -15,6 +15,7 @@ using System.Net; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -3728,24 +3729,6 @@ private void InvokeClassifiedSubscriber(Action subscriber, Daq } } - /// - /// Runs a logging call, swallowing any exception a misbehaving throws. - /// A consumer-supplied logger must never affect device operation — least of all in - /// , whose whole purpose is to isolate frame processing - /// from faults. Mirrors MessageProducer.SafeLog. - /// - private static void SafeLog(Action logAction) - { - try - { - logAction(); - } - catch - { - // A logger that throws is not permitted to take down device operation. - } - } - /// /// Populates the device channels from a protobuf status message. /// diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 0fba2c28..915e48e8 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -12,12 +12,12 @@ using Daqifi.Core.Firmware; using System; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Linq; using System.Net; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -519,31 +519,6 @@ private void RaiseStreamFrameDiscarded(StreamFrameDiscardedEventArgs e) } } - /// - /// Writes a diagnostic line, swallowing anything a misbehaving - /// throws. - /// - /// - /// dispatches to listeners the consumer installed, so it is consumer - /// code and can throw like any other. That matters most in the places that exist purely to - /// isolate the frame pipeline from faults: a listener throwing out of the catch that - /// was containing a bad subscriber would defeat the containment and take down the very - /// frame processing it was protecting. Same reasoning, and the same guarantee, as - /// DaqifiDevice.SafeLog — which is private to the base class, hence this local twin. - /// - /// The diagnostic line to write. - private static void SafeTrace(string message) - { - try - { - Trace.WriteLine(message); - } - catch - { - // A trace listener that throws is not permitted to affect device operation. - } - } - /// /// Raises , isolating the decode pipeline from a subscriber /// exception so a throwing handler cannot skip this frame's per-channel decode (which the diff --git a/src/Daqifi.Core/Device/Discovery/AllTransportsDeviceFinder.cs b/src/Daqifi.Core/Device/Discovery/AllTransportsDeviceFinder.cs index f9f08f75..0c6a96ab 100644 --- a/src/Daqifi.Core/Device/Discovery/AllTransportsDeviceFinder.cs +++ b/src/Daqifi.Core/Device/Discovery/AllTransportsDeviceFinder.cs @@ -1,9 +1,9 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; namespace Daqifi.Core.Device.Discovery; @@ -222,18 +222,6 @@ private async Task> SafeDiscoverAsync(IDeviceFinder fin } } - private static void SafeTrace(string message) - { - try - { - Trace.WriteLine(message); - } - catch - { - // Best-effort logging: a misbehaving TraceListener must never affect discovery. - } - } - private string Identity(IDeviceInfo device) { if (_identitySelector != null) diff --git a/src/Daqifi.Core/Device/Internal/LifecycleGate.cs b/src/Daqifi.Core/Device/Internal/LifecycleGate.cs index 7fb34489..c2b7d0ef 100644 --- a/src/Daqifi.Core/Device/Internal/LifecycleGate.cs +++ b/src/Daqifi.Core/Device/Internal/LifecycleGate.cs @@ -2,6 +2,7 @@ using System; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -337,22 +338,4 @@ private void ReleaseLifecycleLock() // 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. - } - } } diff --git a/src/Daqifi.Core/Device/Internal/OperationSerializer.cs b/src/Daqifi.Core/Device/Internal/OperationSerializer.cs index ec9d852c..5ca0b985 100644 --- a/src/Daqifi.Core/Device/Internal/OperationSerializer.cs +++ b/src/Daqifi.Core/Device/Internal/OperationSerializer.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -659,22 +660,6 @@ internal async Task DrainOutboundQueueAsync(CancellationToken cancellationToken) } } - /// - /// Runs without letting a throwing logger take down an - /// operation — the same isolation DaqifiDevice.SafeLog gives the rest of the device. - /// - private static void SafeLog(Action logAction) - { - try - { - logAction(); - } - catch - { - // A logger that throws is not permitted to take down device operation. - } - } - public void Dispose() { _operationLock.Dispose(); diff --git a/src/Daqifi.Core/Device/Internal/ReconnectSupervisor.cs b/src/Daqifi.Core/Device/Internal/ReconnectSupervisor.cs index c816e54f..7296a3a7 100644 --- a/src/Daqifi.Core/Device/Internal/ReconnectSupervisor.cs +++ b/src/Daqifi.Core/Device/Internal/ReconnectSupervisor.cs @@ -2,6 +2,7 @@ using System; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -444,20 +445,4 @@ private void ReportReconnectStopped(int epoch, int attemptsMade, Exception? last _host.RaiseReconnectFailed(new ReconnectFailedEventArgs(attemptsMade, lastError, wasCanceled)); } - - /// - /// Runs a log call, swallowing anything it throws — a logger is not permitted to take down a - /// reconnect, the same isolation DaqifiDevice.SafeLog gives the rest of the device. - /// - private static void SafeLog(Action logAction) - { - try - { - logAction(); - } - catch - { - // A logger that throws is not permitted to take down device operation. - } - } } diff --git a/src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs b/src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs index 49e43e04..28ee1de2 100644 --- a/src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs +++ b/src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Logging; using System; using System.Collections.Generic; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -326,25 +327,4 @@ private static T GetWithDefault(IList list, int index, T defaultValue) } return defaultValue; } - - /// - /// Runs a logging call, swallowing anything a consumer-supplied - /// throws. Mirrors DaqifiDevice.SafeLog, which is private to that class. - /// - /// - /// A logger that throws must not abort channel population: the warnings guarded here are - /// emitted precisely when the device reported something implausible, so a faulting logger - /// would turn a recoverable bad status frame into a failed population. - /// - private static void SafeLog(Action logAction) - { - try - { - logAction(); - } - catch - { - // A logger that throws is not permitted to affect device operation. - } - } } diff --git a/src/Daqifi.Core/Device/Internal/StreamingSessionController.cs b/src/Daqifi.Core/Device/Internal/StreamingSessionController.cs index 03b0c5b6..a7e9e069 100644 --- a/src/Daqifi.Core/Device/Internal/StreamingSessionController.cs +++ b/src/Daqifi.Core/Device/Internal/StreamingSessionController.cs @@ -1,9 +1,9 @@ using System; -using System.Diagnostics; using System.Threading; using System.Threading.Tasks; using Daqifi.Core.Channel; using Daqifi.Core.Communication.Producers; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -312,20 +312,4 @@ internal Task RestoreSessionSnapshotAsync( } #endregion - - /// - /// Writes a diagnostic line, swallowing anything a misbehaving - /// throws. Byte-identical twin of the device's own private helper. - /// - private static void SafeTrace(string message) - { - try - { - Trace.WriteLine(message); - } - catch - { - // A trace listener that throws is not permitted to affect device operation. - } - } } diff --git a/src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs b/src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs index 25f6bf66..d396116c 100644 --- a/src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs +++ b/src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs @@ -8,6 +8,7 @@ using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; +using static Daqifi.Core.Internal.DiagnosticGuard; #nullable enable @@ -1073,21 +1074,7 @@ private void RestartMessageConsumerAfterSwap() } } - /// - /// Logs through the device's logger without letting a throwing logger take down an exchange — - /// the same isolation DaqifiDevice.SafeLog gives the rest of the device. - /// - private void Log(Action logAction) - { - try - { - logAction(_host.Logger); - } - catch - { - // A logger that throws is not permitted to take down device operation. - } - } + private void Log(Action logAction) => SafeLog(() => logAction(_host.Logger)); /// /// One line the text consumer produced, together with what the outbound writer had already diff --git a/src/Daqifi.Core/Internal/DiagnosticGuard.cs b/src/Daqifi.Core/Internal/DiagnosticGuard.cs new file mode 100644 index 00000000..afc8b7ae --- /dev/null +++ b/src/Daqifi.Core/Internal/DiagnosticGuard.cs @@ -0,0 +1,43 @@ +using System.Diagnostics; + +namespace Daqifi.Core.Internal; + +/// +/// Isolates diagnostic side-effects from device and transport operation. A consumer-supplied +/// logger or must never take down a connect, reconnect, drop path, +/// or frame pipeline. +/// +internal static class DiagnosticGuard +{ + /// + /// Runs a logging call (or any similarly-isolated side-effect), swallowing anything it throws. + /// + /// The logging call or isolated side-effect to run. + internal static void SafeLog(Action action) + { + try + { + action(); + } + catch + { + // A logger that throws is not permitted to take down device operation. + } + } + + /// + /// Writes a diagnostic line, swallowing anything a misbehaving throws. + /// + /// The diagnostic line to write. + internal static void SafeTrace(string message) => SafeLog(() => Trace.WriteLine(message)); + + /// + /// Composes and writes a diagnostic line, swallowing both a throwing listener and a message + /// factory that throws (for example on a consumer-supplied exception). + /// + /// + /// Factory for the diagnostic line. Invoked inside the guard so a throwing + /// cannot escape the drop path. + /// + internal static void SafeTrace(Func message) => SafeLog(() => Trace.WriteLine(message())); +} From fdaddbe3b109ea8063cb2de8d1dd9dda9ca01452 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sun, 20 Sep 2026 15:22:08 -1000 Subject: [PATCH 2/4] refactor(core): keep the call-site rationale the extraction dropped Review follow-up on #763. No behaviour change. The twelve deleted helpers carried the reasoning for the containment, not just the containment itself, and some of it had no home in the shared type: - Both transports now say why the trace line is composed inside a lambda. Without that, `SafeTrace(() => $"...")` reads as idle indirection and the obvious "simplification" back to `SafeTrace($"...")` silently reopens #494 -- a subscriber exception whose ToString() throws escaping the catch that was containing it. - DiagnosticGuard's remarks state the contract the empty catch depends on: purely diagnostic work only, nothing the device's correctness rests on, and no logging from inside the catch (#98). SafeTrace_WritesWithoutThrowing only asserted that nothing was thrown, which is equally true of a guard that quietly writes nothing. Replaced with two tests that capture the trace and assert the line actually lands, through the parallel-safe synchronized-listener pattern DropPathSubscriberIsolationTests established. Co-Authored-By: Claude Opus 5 --- .../Internal/DiagnosticGuardTests.cs | 49 +++++++++++++++++-- .../Transport/SerialStreamTransport.cs | 6 +++ .../Transport/TcpStreamTransport.cs | 3 +- src/Daqifi.Core/Internal/DiagnosticGuard.cs | 20 +++++++- 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs b/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs index c6ead6ed..2a7866ba 100644 --- a/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs +++ b/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using Daqifi.Core.Internal; namespace Daqifi.Core.Tests.Internal; @@ -21,17 +22,57 @@ public void SafeLog_SwallowsThrownExceptions() } [Fact] - public void SafeTrace_WritesWithoutThrowing() + public void SafeTrace_ActuallyWritesTheLine() { - var ex = Record.Exception(() => DiagnosticGuard.SafeTrace("diagnostic")); - Assert.Null(ex); + // "It didn't throw" is also true of a guard that quietly writes nothing, and a + // diagnostic helper that silently stopped tracing is exactly the regression nobody + // would notice. The listener is synchronized and asserted with Contains, so traffic + // from tests running in parallel can neither corrupt it nor fail this -- the same + // arrangement DropPathSubscriberIsolationTests uses, and the reason a *throwing* + // listener is not installed here: Trace.Listeners is process-global. + var marker = $"diagnostic-{Guid.NewGuid():N}"; + var traced = CaptureTrace(() => DiagnosticGuard.SafeTrace(marker)); + + Assert.Contains(marker, traced, StringComparison.Ordinal); } [Fact] - public void SafeTrace_SwallowsAThrowingMessageFactory() + public void SafeTrace_ComposesTheLineInsideTheGuard_SoAThrowingFactoryCannotEscape() { + // The overload exists precisely so a consumer-supplied exception whose ToString() + // throws cannot escape the catch that is containing it (issue #494). Composing + // eagerly at the call site would defeat that. var ex = Record.Exception(() => DiagnosticGuard.SafeTrace(static () => throw new InvalidOperationException("cannot render"))); + Assert.Null(ex); } + + [Fact] + public void SafeTrace_FactoryOverload_AlsoWritesTheLine() + { + var marker = $"composed-{Guid.NewGuid():N}"; + var traced = CaptureTrace(() => DiagnosticGuard.SafeTrace(() => marker)); + + Assert.Contains(marker, traced, StringComparison.Ordinal); + } + + private static string CaptureTrace(Action act) + { + var captured = new StringWriter(); + var listener = new TextWriterTraceListener(TextWriter.Synchronized(captured)); + Trace.Listeners.Add(listener); + try + { + act(); + Trace.Flush(); + } + finally + { + Trace.Listeners.Remove(listener); + listener.Dispose(); + } + + return captured.ToString(); + } } diff --git a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs index 26b8eb0d..cb5f6183 100644 --- a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs +++ b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs @@ -675,6 +675,12 @@ private void HandleConnectionLost(Exception error) // disturbing them. A DaqifiDevice surfaces its own StatusChanged subscriber failures on // ErrorOccurred; this transport carries no logger, so a consumer working against a bare // transport gets the same best-effort trace DeviceFinderBase gives its event raises. + // + // The lambda is load-bearing, not decoration: `ex` came out of consumer code too, so + // rendering it is no safer than raising to it. Composing the line eagerly — SafeTrace($"...") + // — would let a throwing ToString() escape the very catch that is containing the + // subscriber, and the watchdog/reader thread would be disrupted anyway (issue #494, + // pinned by DropPathSubscriberIsolationTests). SafeTrace(() => $"[{nameof(SerialStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {ex}"); } diff --git a/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs b/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs index 2b2a09c0..defc9f26 100644 --- a/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs +++ b/src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs @@ -416,7 +416,8 @@ private void HandleConnectionLost(Exception error) // already been taken out of their fields, so if this unwound past the dispose below, // Disconnect() and Dispose() would both find null and skip them too, leaking the socket // for the life of the process. Not rethrown, and traced best-effort because this - // transport carries no logger: see SerialStreamTransport.HandleConnectionLost. + // transport carries no logger: see SerialStreamTransport.HandleConnectionLost, which + // also explains why the line is composed inside the lambda rather than eagerly. SafeTrace(() => $"[{nameof(TcpStreamTransport)}] a {nameof(StatusChanged)} subscriber threw while a dropped connection was being reported: {ex}"); } diff --git a/src/Daqifi.Core/Internal/DiagnosticGuard.cs b/src/Daqifi.Core/Internal/DiagnosticGuard.cs index afc8b7ae..2d3c32b4 100644 --- a/src/Daqifi.Core/Internal/DiagnosticGuard.cs +++ b/src/Daqifi.Core/Internal/DiagnosticGuard.cs @@ -7,12 +7,28 @@ namespace Daqifi.Core.Internal; /// logger or must never take down a connect, reconnect, drop path, /// or frame pipeline. /// +/// +/// +/// Replaces a dozen byte-identical SafeLog / SafeTrace twins that had accumulated +/// across the device, producer, and transport types. +/// +/// +/// The empty catch is deliberate, and it is the reason this type is narrow. Swallowing an +/// exception is only defensible when the work being swallowed is purely diagnostic — +/// when dropping it costs a log line and nothing else. That is the whole contract here: pass a +/// logger call or a write, nothing that the device's own correctness depends +/// on. Nor does the guard log the failure it caught: logging from inside a catch was considered +/// and rejected for this codebase (issue #98), and would in any case mean calling the very +/// logger that just threw. +/// +/// internal static class DiagnosticGuard { /// - /// Runs a logging call (or any similarly-isolated side-effect), swallowing anything it throws. + /// Runs a logging call, swallowing anything it throws. See the type-level remarks for what + /// may and may not be passed here. /// - /// The logging call or isolated side-effect to run. + /// The logging call to run. internal static void SafeLog(Action action) { try From 79429cc6e66b8ef6717be015d71861d15adf0fdd Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sun, 20 Sep 2026 15:58:14 -1000 Subject: [PATCH 3/4] docs(core): state DiagnosticGuard's real contract, events included The contract added in fdaddbe said the guarded work must be purely diagnostic -- a logger call or a Trace write, nothing else. That is wrong about this codebase: SafeLog also guards best-effort event raises (MessageProducer's SendFailed, DaqifiDevice's SendFailed and ErrorOccurred), where the point is that a throwing subscriber must not take down the producer's background thread or the reader loop. A maintainer following the too-narrow wording could have removed that isolation as out of contract. Restated as what actually holds: what may be passed is a best-effort notification OUT to consumer code -- logger, TraceListener, or a best-effort event raise -- where the only thing lost when it throws is that one notification. What may not be passed is anything the device's own state or correctness depends on. Documentation only; no code change. Co-Authored-By: Claude Opus 5 --- src/Daqifi.Core/Internal/DiagnosticGuard.cs | 36 ++++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/Daqifi.Core/Internal/DiagnosticGuard.cs b/src/Daqifi.Core/Internal/DiagnosticGuard.cs index 2d3c32b4..122652ab 100644 --- a/src/Daqifi.Core/Internal/DiagnosticGuard.cs +++ b/src/Daqifi.Core/Internal/DiagnosticGuard.cs @@ -3,9 +3,9 @@ namespace Daqifi.Core.Internal; /// -/// Isolates diagnostic side-effects from device and transport operation. A consumer-supplied -/// logger or must never take down a connect, reconnect, drop path, -/// or frame pipeline. +/// Isolates best-effort calls out to consumer code from device and transport operation. A +/// consumer-supplied logger, , or event subscriber must never take +/// down a connect, reconnect, drop path, or frame pipeline. /// /// /// @@ -13,22 +13,34 @@ namespace Daqifi.Core.Internal; /// across the device, producer, and transport types. /// /// -/// The empty catch is deliberate, and it is the reason this type is narrow. Swallowing an -/// exception is only defensible when the work being swallowed is purely diagnostic — -/// when dropping it costs a log line and nothing else. That is the whole contract here: pass a -/// logger call or a write, nothing that the device's own correctness depends -/// on. Nor does the guard log the failure it caught: logging from inside a catch was considered -/// and rejected for this codebase (issue #98), and would in any case mean calling the very +/// The empty catch is deliberate, and the contract that makes it defensible is worth +/// stating precisely, because it is narrower than "swallow exceptions" and wider than "logging". +/// What may be passed here is a best-effort notification out to consumer code — an +/// call, a write, or the +/// raise of a best-effort event such as SendFailed or ErrorOccurred — where the +/// only thing lost when it throws is that one notification. Consumer code is entitled to +/// misbehave; the producer's background thread, the reader loop, and the decode path are not +/// entitled to die because it did. +/// +/// +/// What may not be passed is anything the device's own state or correctness depends on. +/// A call whose failure has to change what happens next does not belong behind a guard that +/// cannot report it. +/// +/// +/// Nor does the guard log the failure it caught: logging from inside a catch was considered and +/// rejected for this codebase (issue #98), and would in any case often mean calling the very /// logger that just threw. /// /// internal static class DiagnosticGuard { /// - /// Runs a logging call, swallowing anything it throws. See the type-level remarks for what - /// may and may not be passed here. + /// Runs a best-effort notification out to consumer code — a logging call, or the raise of an + /// event whose subscribers must not be able to take down the caller — swallowing anything it + /// throws. See the type-level remarks for what may and may not be passed here. /// - /// The logging call to run. + /// The logging call or best-effort event raise to run. internal static void SafeLog(Action action) { try From fc5785f434095c4cb516f511577949329d9c0227 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sun, 27 Sep 2026 10:43:03 -0600 Subject: [PATCH 4/4] docs(core): state DiagnosticGuard's purpose, not its history The remarks called the twelve helpers it replaced byte-identical. They were not: four shapes (Action, string, Exception, and an instance Action), and the transports composed their own line. Replace the change-log sentence with the guidance a future reader needs, and widen the catch comment from "logger" to consumer code, matching the documented contract (event raises too). Comment-only. Co-Authored-By: Claude Opus 5.5 --- src/Daqifi.Core/Internal/DiagnosticGuard.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Daqifi.Core/Internal/DiagnosticGuard.cs b/src/Daqifi.Core/Internal/DiagnosticGuard.cs index 122652ab..9fc3dfe5 100644 --- a/src/Daqifi.Core/Internal/DiagnosticGuard.cs +++ b/src/Daqifi.Core/Internal/DiagnosticGuard.cs @@ -9,8 +9,8 @@ namespace Daqifi.Core.Internal; /// /// /// -/// Replaces a dozen byte-identical SafeLog / SafeTrace twins that had accumulated -/// across the device, producer, and transport types. +/// Use this rather than a private try/catch copy in each type, so the guarantee and the contract +/// below live in one place. /// /// /// The empty catch is deliberate, and the contract that makes it defensible is worth @@ -49,7 +49,7 @@ internal static void SafeLog(Action action) } catch { - // A logger that throws is not permitted to take down device operation. + // Consumer code that throws is not permitted to take down device operation. } }