refactor(core): extract shared SafeLog/SafeTrace helper

src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs

@@ -0,0 +1,78 @@
+using System.Diagnostics;
+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_ActuallyWritesTheLine()
+    {
+        // "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_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();
+    }
+}

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;
 
@@ -371,23 +372,6 @@ private void ProcessMessages()
         }
     }
 
-    /// <summary>
-    /// 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.
-    /// </summary>
-    private static void SafeLog(Action logAction)
-    {
-        try
-        {
-            logAction();
-        }
-        catch
-        {
-            // A logger that throws is not permitted to take down the producer.
-        }
-    }
-
     /// <summary>
     /// Marks the producer idle if the queue is still empty after a drain.
     /// </summary>

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,14 @@ 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);
+            //
+            // 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}");
         }
         finally
         {
@@ -689,40 +697,6 @@ private void HandleConnectionLost(Exception error)
         }
     }
 
-    /// <summary>
-    /// Writes the diagnostic line for a <see cref="StatusChanged"/> subscriber failure, swallowing
-    /// anything a misbehaving <see cref="System.Diagnostics.TraceListener"/> throws.
-    /// </summary>
-    /// <remarks>
-    /// <see cref="System.Diagnostics.Trace"/> dispatches to listeners the consumer installed, so it
-    /// is consumer code and can throw like any other. A listener throwing out of the <c>catch</c>
-    /// that was containing a bad subscriber would defeat the containment and cost the port handle
-    /// anyway. Same guarantee as <c>DeviceFinderBase.RaiseIsolated</c> and
-    /// <c>DaqifiStreamingDevice.SafeTrace</c>; this transport has no <c>ILogger</c>, hence the local
-    /// twin rather than a shared one.
-    /// <para>
-    /// The message is composed <em>inside</em> the guard rather than passed in ready-made, because
-    /// <paramref name="subscriberFailure"/> came out of consumer code too: rendering an exception
-    /// whose <see cref="object.ToString"/> or <see cref="Exception.Message"/> throws would otherwise
-    /// escape the <c>catch</c> from the interpolation itself and leave the drop path exactly as
-    /// disrupted as an unguarded raise.
-    /// </para>
-    /// </remarks>
-    /// <param name="subscriberFailure">The exception the subscriber threw.</param>
-    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.
-        }
-    }
-
     /// <summary>
     /// Raises the StatusChanged event.
     /// </summary>

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;
 
@@ -415,8 +416,10 @@ 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.
-            SafeTrace(ex);
+            // 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}");
         }
         finally
         {
@@ -432,27 +435,6 @@ private void HandleConnectionLost(Exception error)
         }
     }
 
-    /// <summary>
-    /// Writes the diagnostic line for a <see cref="StatusChanged"/> subscriber failure, swallowing
-    /// anything a misbehaving <see cref="System.Diagnostics.TraceListener"/> throws. See
-    /// <c>SerialStreamTransport.SafeTrace</c> for why the trace itself — and the composition of its
-    /// message from a consumer-supplied exception — has to be contained.
-    /// </summary>
-    /// <param name="subscriberFailure">The exception the subscriber threw.</param>
-    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.
-        }
-    }
-
     /// <summary>
     /// Raises the StatusChanged event.
     /// </summary>

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<DaqifiOutMessage> subscriber, Daq
         }
     }
 
-    /// <summary>
-    /// Runs a logging call, swallowing any exception a misbehaving <see cref="ILogger"/> throws.
-    /// A consumer-supplied logger must never affect device operation — least of all in
-    /// <see cref="RaiseClassifiedEvent"/>, whose whole purpose is to isolate frame processing
-    /// from faults. Mirrors <c>MessageProducer.SafeLog</c>.
-    /// </summary>
-    private static void SafeLog(Action logAction)
-    {
-        try
-        {
-            logAction();
-        }
-        catch
-        {
-            // A logger that throws is not permitted to take down device operation.
-        }
-    }
-
     /// <summary>
     /// Populates the device channels from a protobuf status message.
     /// </summary>

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)
         }
     }
 
-    /// <summary>
-    /// Writes a diagnostic line, swallowing anything a misbehaving <see cref="TraceListener"/>
-    /// throws.
-    /// </summary>
-    /// <remarks>
-    /// <see cref="Trace"/> 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 <c>catch</c> 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
-    /// <c>DaqifiDevice.SafeLog</c> — which is private to the base class, hence this local twin.
-    /// </remarks>
-    /// <param name="message">The diagnostic line to write.</param>
-    private static void SafeTrace(string message)
-    {
-        try
-        {
-            Trace.WriteLine(message);
-        }
-        catch
-        {
-            // A trace listener that throws is not permitted to affect device operation.
-        }
-    }
-
     /// <summary>
     /// Raises <see cref="GapDetected"/>, isolating the decode pipeline from a subscriber
     /// exception so a throwing handler cannot skip this frame's per-channel decode (which the

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<IEnumerable<IDeviceInfo>> 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)

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.
         }
     }
-
-    /// <summary>
-    /// Runs a logging call, swallowing any exception a misbehaving <see cref="ILogger"/> 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
-    /// <c>DaqifiDevice.SafeLog</c>.
-    /// </summary>
-    private static void SafeLog(Action logAction)
-    {
-        try
-        {
-            logAction();
-        }
-        catch
-        {
-            // A logger that throws is not permitted to take down device operation.
-        }
-    }
 }

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)
         }
     }
 
-    /// <summary>
-    /// Runs <paramref name="logAction"/> without letting a throwing logger take down an
-    /// operation — the same isolation <c>DaqifiDevice.SafeLog</c> gives the rest of the device.
-    /// </summary>
-    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();