diff --git a/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs b/src/Daqifi.Core.Tests/Internal/DiagnosticGuardTests.cs new file mode 100644 index 00000000..2a7866ba --- /dev/null +++ b/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(); + } +} diff --git a/src/Daqifi.Core/Communication/Producers/MessageProducer.cs b/src/Daqifi.Core/Communication/Producers/MessageProducer.cs index d2630967..c458273a 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; @@ -371,23 +372,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. - } - } - /// /// Marks the producer idle if the queue is still empty after a drain. /// diff --git a/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs b/src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs index 82f60a8f..cb5f6183 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,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) } } - /// - /// 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..defc9f26 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; @@ -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) } } - /// - /// 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 cf5be802..b8b335e0 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..9fc3dfe5 --- /dev/null +++ b/src/Daqifi.Core/Internal/DiagnosticGuard.cs @@ -0,0 +1,71 @@ +using System.Diagnostics; + +namespace Daqifi.Core.Internal; + +/// +/// 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. +/// +/// +/// +/// 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 +/// 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 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 or best-effort event raise to run. + internal static void SafeLog(Action action) + { + try + { + action(); + } + catch + { + // Consumer code 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())); +}