dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(client-cli): resolve Medium code-review findings (Client.CLI-001, Client.CLI-005)

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 08:08:25 -04:00
parent 9f5a5c9997
commit aa142f6dd4
4 changed files with 98 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
code-reviews/Client.CLI/findings.md
View File

@@ -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<T>` 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<string>` 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

54
src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AlarmsCommand.cs
View File

@@ -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<string>(
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.");
}

11
src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/HistoryReadCommand.cs
View File

@@ -1,3 +1,4 @@
using System.Globalization;
using CliFx.Attributes;
using CliFx.Infrastructure;
using Opc.Ua;
@@ -27,13 +28,13 @@ public class HistoryReadCommand : CommandBase
/// <summary>
/// Gets the optional history start time string supplied by the operator.
/// </summary>
[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; }
/// <summary>
/// Gets the optional history end time string supplied by the operator.
/// </summary>
[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; }
/// <summary>
@@ -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<DataValue> values;

56
src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/SubscribeCommand.cs
View File

@@ -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<string, byte>();
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<string>(
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<string>();
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}");