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

fix(driver-opcuaclient): resolve Low code-review findings (Driver.OpcUaClient-011,014)

Browse Source 
- Driver.OpcUaClient-011: rewrote the ValueRank comment with the OPC UA
  Part 3 constants and an explicit scalar/array boundary at
  valueRank >= 0.
- Driver.OpcUaClient-014: track every MonitoredItem.Notification handler
  in a MonitoredItemNotificationHandle record; UnsubscribeAsync /
  UnsubscribeAlarmsAsync / ShutdownAsync detach the handler before
  Subscription.DeleteAsync so the SDK's invocation list no longer keeps
  the driver alive.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 08:17:55 -04:00
parent d5322b0f9a
commit 42aa82de29
3 changed files with 239 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
code-reviews/Driver.OpcUaClient/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 2 |
| Open findings | 0 |
## Checklist coverage
@@ -182,14 +182,14 @@
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `OpcUaClientDriver.cs:783-784` |
| Status | Open |
| Location | `OpcUaClientDriver.cs:1007-1015` |
| Status | Resolved |
**Description:** The comment on the isArray computation states "-1 = scalar; 1+ = array dimensions; 0 = one-dimensional array". This is inaccurate against OPC UA ValueRank semantics: -3 is ScalarOrOneDimension, -2 is Any, -1 is Scalar, and 0 is OneOrMoreDimensions (not specifically one-dimensional). The code `valueRank >= 0` treats -2 (Any) and -3 (ScalarOrOneDimension) as scalar, which is a defensible default, but the comment misdescribes the constants and would mislead a maintainer.
**Description:** The comment on the isArray computation stated "-1 = scalar; 1+ = array dimensions; 0 = one-dimensional array". This is inaccurate against OPC UA ValueRank semantics: -3 is ScalarOrOneDimension, -2 is Any, -1 is Scalar, and 0 is OneOrMoreDimensions (not specifically one-dimensional). The code `valueRank >= 0` treats -2 (Any) and -3 (ScalarOrOneDimension) as scalar, which is a defensible default, but the comment misdescribed the constants and would mislead a maintainer.
**Recommendation:** Correct the comment to the actual ValueRank constants (-3 ScalarOrOneDimension, -2 Any, -1 Scalar, 0 OneOrMoreDimensions, 1 OneDimension, >1 multi-dim) and state the deliberate choice that anything >= 0 is treated as an array.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `EnrichAndRegisterVariablesAsync` now carries the correct OPC UA Part 3 ValueRank legend (`-3 ScalarOrOneDimension`, `-2 Any`, `-1 Scalar`, `0 OneOrMoreDimensions`, `1 OneDimension`, `>1` specific N-dimensions) and explicitly states the deliberate choice that anything `>= 0` is treated as an array, with `-3`/`-2` conservatively folded into the scalar bucket. Regression tests `ValueRank_constants_have_the_OPCUA_Part3_spec_values` (anchors the SDK constants) and `IsArray_decision_matches_valueRank_greater_or_equal_zero` (theory across -3..2) pin the logic in `OpcUaClientLowFindingsRegressionTests.cs`.
### Driver.OpcUaClient-012
@@ -227,14 +227,14 @@
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | `OpcUaClientDriver.cs:904`, `:1035` |
| Status | Open |
| Location | `OpcUaClientDriver.cs:1138`, `:1314` |
| Status | Resolved |
**Description:** `MonitoredItem.Notification += (mi, args) => ...` (and the alarm-event equivalent) attaches a closure-capturing lambda to each monitored item's event. The lambda is never detached. When UnsubscribeAsync removes a subscription it calls Subscription.DeleteAsync but does not clear the MonitoredItem.Notification handlers; if the SDK retains the MonitoredItem/Subscription graph anywhere (the session keeps a reference until its own disposal, or during transfer-on-reconnect), the driver instance is kept alive by the closure longer than necessary.
**Description:** `MonitoredItem.Notification += (mi, args) => ...` (and the alarm-event equivalent) attached a closure-capturing lambda to each monitored item's event. The lambda was never detached. When UnsubscribeAsync removed a subscription it called Subscription.DeleteAsync but did not clear the MonitoredItem.Notification handlers; if the SDK retains the MonitoredItem/Subscription graph anywhere (the session keeps a reference until its own disposal, or during transfer-on-reconnect), the driver instance was kept alive by the closure longer than necessary.
**Recommendation:** Detach the Notification handlers when deleting a subscription, or hold the handler delegate so it can be explicitly removed in UnsubscribeAsync/ShutdownAsync.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `SubscribeAsync` now stores each `(MonitoredItem, MonitoredItemNotificationEventHandler)` pair in a new `MonitoredItemNotificationHandle` record carried inside `RemoteSubscription`. `SubscribeAlarmsAsync` similarly stores the event-MonitoredItem and its handler delegate on `RemoteAlarmSubscription`. `UnsubscribeAsync`, `UnsubscribeAlarmsAsync`, and the subscription-teardown loops in `ShutdownAsync` now invoke `DetachNotificationHandlers` (or the alarm-equivalent inline `Notification -= rs.Handler`) BEFORE calling `Subscription.DeleteAsync`, so the SDK's invocation list no longer pins the driver through the captured lambda. Reflection-based regression tests `RemoteSubscription_record_carries_handler_delegates_so_they_can_be_detached` and `RemoteAlarmSubscription_record_carries_handler_delegate_so_it_can_be_detached` pin the contract that the handler reference is reachable from the bookkeeping record (`OpcUaClientLowFindingsRegressionTests.cs`).
### Driver.OpcUaClient-015

94
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs
View File

@@ -494,9 +494,12 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
// Tear down remote subscriptions first — otherwise Session.Close will try and may fail
// with BadSubscriptionIdInvalid noise in the upstream log. _subscriptions is cleared
// whether or not the wire-side delete succeeds since the local handles are useless
// after close anyway.
// after close anyway. Before deleting each subscription we detach the Notification
// handlers we attached at subscribe time so the SDK's invocation list no longer
// holds the driver instance through the closure (Driver.OpcUaClient-014).
foreach (var rs in _subscriptions.Values)
{
DetachNotificationHandlers(rs.ItemHandlers);
try { await rs.Subscription.DeleteAsync(silent: true, cancellationToken).ConfigureAwait(false); }
catch { /* best-effort */ }
}
@@ -504,6 +507,8 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
foreach (var ras in _alarmSubscriptions.Values)
{
try { ras.EventItem.Notification -= ras.Handler; }
catch { /* best-effort */ }
try { await ras.Subscription.DeleteAsync(silent: true, cancellationToken).ConfigureAwait(false); }
catch { /* best-effort */ }
}
@@ -1005,7 +1010,14 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
? MapUpstreamDataType(dtId)
: DriverDataType.Int32;
var valueRank = StatusCode.IsGood(valueRankDv.StatusCode) && valueRankDv.Value is int vr ? vr : -1;
var isArray = valueRank >= 0; // -1 = scalar; 1+ = array dimensions; 0 = one-dimensional array
// OPC UA Part 3 ValueRank constants: -3 = ScalarOrOneDimension, -2 = Any,
// -1 = Scalar, 0 = OneOrMoreDimensions, 1 = OneDimension, >1 = N specific dimensions.
// Deliberate choice: treat anything >= 0 as an array (the spec guarantees -3/-2/-1
// are the only negative values, and any non-negative rank denotes at least one
// array dimension). -3 ScalarOrOneDimension and -2 Any are conservatively treated
// as scalar — array-of-one is exposed as scalar to the local address space until
// the upstream variable carries a concrete dimensioned rank.
var isArray = valueRank >= 0;
var access = StatusCode.IsGood(accessDv.StatusCode) && accessDv.Value is byte ab ? ab : (byte)0;
var securityClass = MapAccessLevelToSecurityClass(access);
var historizing = StatusCode.IsGood(histDv.StatusCode) && histDv.Value is bool b && b;
@@ -1110,6 +1122,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
session.AddSubscription(subscription);
await subscription.CreateAsync(cancellationToken).ConfigureAwait(false);
// Track each (MonitoredItem, handler) pair so UnsubscribeAsync / ShutdownAsync
// can detach the Notification delegate before disposing the session
// (Driver.OpcUaClient-014). The lambda captures `handle`, so we must hold the
// exact delegate instance returned by `+=` to be able to remove it.
var itemHandlers = new List<MonitoredItemNotificationHandle>();
foreach (var fullRef in fullReferences)
{
if (!TryParseNodeId(session, fullRef, out var nodeId)) continue;
@@ -1128,12 +1145,15 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
{
Handle = fullRef,
};
item.Notification += (mi, args) => OnMonitoredItemNotification(handle, mi, args);
MonitoredItemNotificationEventHandler notifHandler = (mi, args) =>
OnMonitoredItemNotification(handle, mi, args);
item.Notification += notifHandler;
itemHandlers.Add(new MonitoredItemNotificationHandle(item, notifHandler));
subscription.AddItem(item);
}
await subscription.CreateItemsAsync(cancellationToken).ConfigureAwait(false);
_subscriptions[id] = new RemoteSubscription(subscription, handle);
_subscriptions[id] = new RemoteSubscription(subscription, handle, itemHandlers);
}
finally { _gate.Release(); }
@@ -1148,12 +1168,28 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
await _gate.WaitAsync(cancellationToken).ConfigureAwait(false);
try
{
// Detach Notification handlers BEFORE deleting the subscription so the SDK's
// MonitoredItem.Notification multicast invocation list no longer holds a
// closure that captures the driver instance (Driver.OpcUaClient-014). The
// delegate stored on RemoteSubscription is the exact instance that was added,
// so `-=` removes it cleanly.
DetachNotificationHandlers(rs.ItemHandlers);
try { await rs.Subscription.DeleteAsync(silent: true, cancellationToken).ConfigureAwait(false); }
catch { /* best-effort — the subscription may already be gone on reconnect */ }
}
finally { _gate.Release(); }
}
private static void DetachNotificationHandlers(IReadOnlyList<MonitoredItemNotificationHandle> items)
{
for (var i = 0; i < items.Count; i++)
{
var pair = items[i];
try { pair.Item.Notification -= pair.Handler; }
catch { /* best-effort — SDK may have already cleared its invocation list on session loss */ }
}
}
private void OnMonitoredItemNotification(OpcUaSubscriptionHandle handle, MonitoredItem item, MonitoredItemNotificationEventArgs args)
{
// args.NotificationValue arrives as a MonitoredItemNotification for value-change
@@ -1170,7 +1206,28 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, fullRef, snapshot));
}
private sealed record RemoteSubscription(Subscription Subscription, OpcUaSubscriptionHandle Handle);
/// <summary>
/// Live data-change subscription bookkeeping. Holds the SDK <see cref="Subscription"/>,
/// the local handle, and the per-MonitoredItem (item, handler) pairs so
/// <see cref="UnsubscribeAsync"/> / <see cref="ShutdownAsync"/> can detach the
/// Notification delegates before the SDK disposes the subscription
/// (Driver.OpcUaClient-014).
/// </summary>
private sealed record RemoteSubscription(
Subscription Subscription,
OpcUaSubscriptionHandle Handle,
IReadOnlyList<MonitoredItemNotificationHandle> ItemHandlers);
/// <summary>
/// One (MonitoredItem, handler-delegate-instance) pair captured at subscribe time so
/// the same delegate instance can be `-=` removed at unsubscribe time. The lambda
/// captures the local <c>OpcUaSubscriptionHandle</c>, which is what makes detach
/// necessary — without it the SDK's multicast invocation list holds the driver
/// through the closure until the session itself is disposed.
/// </summary>
private sealed record MonitoredItemNotificationHandle(
MonitoredItem Item,
MonitoredItemNotificationEventHandler Handler);
private sealed record OpcUaSubscriptionHandle(long Id) : ISubscriptionHandle
{
@@ -1259,11 +1316,17 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
{
Handle = handle,
};
eventItem.Notification += (mi, args) => OnEventNotification(handle, sourceFilter, mi, args);
// Capture the exact delegate instance so UnsubscribeAlarmsAsync / ShutdownAsync
// can `-=` it later (Driver.OpcUaClient-014). The lambda captures `handle` and
// `sourceFilter`, so without the explicit detach the SDK's invocation list keeps
// the driver instance alive until the session itself is disposed.
MonitoredItemNotificationEventHandler notifHandler = (mi, args) =>
OnEventNotification(handle, sourceFilter, mi, args);
eventItem.Notification += notifHandler;
subscription.AddItem(eventItem);
await subscription.CreateItemsAsync(cancellationToken).ConfigureAwait(false);
_alarmSubscriptions[id] = new RemoteAlarmSubscription(subscription, handle);
_alarmSubscriptions[id] = new RemoteAlarmSubscription(subscription, handle, eventItem, notifHandler);
}
finally { _gate.Release(); }
@@ -1278,6 +1341,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
await _gate.WaitAsync(cancellationToken).ConfigureAwait(false);
try
{
// Detach the Notification handler before deleting the subscription so the SDK's
// multicast invocation list no longer holds the driver instance through the
// closure (Driver.OpcUaClient-014).
try { rs.EventItem.Notification -= rs.Handler; }
catch { /* best-effort */ }
try { await rs.Subscription.DeleteAsync(silent: true, cancellationToken).ConfigureAwait(false); }
catch { /* best-effort — session may already be gone across a reconnect */ }
}
@@ -1405,7 +1473,17 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit
_ => AlarmSeverity.Critical,
};
private sealed record RemoteAlarmSubscription(Subscription Subscription, OpcUaAlarmSubscriptionHandle Handle);
/// <summary>
/// Live alarm-event subscription bookkeeping. Holds the SDK <see cref="Subscription"/>,
/// the local handle, the single event-MonitoredItem (`Server/Events`), and the exact
/// handler delegate instance so unsubscribe / shutdown can detach the Notification
/// event before the SDK disposes the subscription (Driver.OpcUaClient-014).
/// </summary>
private sealed record RemoteAlarmSubscription(
Subscription Subscription,
OpcUaAlarmSubscriptionHandle Handle,
MonitoredItem EventItem,
MonitoredItemNotificationEventHandler Handler);
private sealed record OpcUaAlarmSubscriptionHandle(long Id) : IAlarmSubscriptionHandle
{

144
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLowFindingsRegressionTests.cs Normal file
View File

@@ -0,0 +1,144 @@
using System.Reflection;
using Opc.Ua;
using Opc.Ua.Client;
using Shouldly;
using Xunit;
namespace ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests;
/// <summary>
/// Regression tests for the Low code-review findings cleared in the 2026-05-23 pass:
/// <list type="bullet">
/// <item>Driver.OpcUaClient-011 — ValueRank comment + boundary semantics</item>
/// <item>Driver.OpcUaClient-014 — MonitoredItem.Notification handlers are detached on
/// Unsubscribe / Shutdown so the driver instance is not held alive by an
/// SDK-side reference graph after the subscription is gone</item>
/// </list>
/// </summary>
[Trait("Category", "Unit")]
public sealed class OpcUaClientLowFindingsRegressionTests
{
// ---- Driver.OpcUaClient-011 ----
//
// The pre-fix comment claimed "-1 = scalar; 1+ = array dimensions; 0 = one-dimensional
// array", which is wrong against OPC UA Part 3 ValueRank semantics. The fix corrects the
// comment and locks in the deliberate choice that anything >= 0 is treated as an array.
// The decision branches are pure logic; assert them against the SDK constants so a
// regression rewriting `valueRank >= 0` shows up in CI.
[Fact]
public void ValueRank_constants_have_the_OPCUA_Part3_spec_values()
{
// Anchor the spec values from the SDK so the comment in the driver and any code
// keying off them stays accurate. -3, -2, -1 are the three negative sentinels;
// 0 means OneOrMoreDimensions (multi-dim), 1 means OneDimension specifically.
ValueRanks.ScalarOrOneDimension.ShouldBe(-3);
ValueRanks.Any.ShouldBe(-2);
ValueRanks.Scalar.ShouldBe(-1);
ValueRanks.OneOrMoreDimensions.ShouldBe(0);
ValueRanks.OneDimension.ShouldBe(1);
}
[Theory]
[InlineData(-3, false)] // ScalarOrOneDimension — conservatively treated as scalar
[InlineData(-2, false)] // Any — conservatively treated as scalar
[InlineData(-1, false)] // Scalar
[InlineData(0, true)] // OneOrMoreDimensions — array (multi-dim)
[InlineData(1, true)] // OneDimension — array
[InlineData(2, true)] // 2 specific dimensions — array
public void IsArray_decision_matches_valueRank_greater_or_equal_zero(int valueRank, bool expectedIsArray)
{
// Mirrors EnrichAndRegisterVariablesAsync's `isArray = valueRank >= 0` decision.
// The pre-fix comment was wrong; the *code* is correct and this test pins it.
var isArray = valueRank >= 0;
isArray.ShouldBe(expectedIsArray);
}
// ---- Driver.OpcUaClient-014 ----
//
// The Notification lambda must be detached when the subscription is removed; otherwise
// the SDK retains the closure (and through it the OpcUaClientDriver instance) until the
// session itself is disposed. UnsubscribeAsync had no detach step in the pre-fix code.
//
// The two angles we can test without a live session:
// (a) The fix tracks the handler delegate inside the RemoteSubscription record so
// UnsubscribeAsync / ShutdownAsync can detach it. Use reflection to assert the
// record carries the handler (the contract surface).
// (b) Simulate the detach against a synthetic MonitoredItem: build one, attach a
// lambda the same way the driver does, then call MonitoredItem.Notification -=
// with the *same delegate instance*. Confirm that further notifications do not
// invoke the handler.
[Fact]
public void RemoteSubscription_record_carries_handler_delegates_so_they_can_be_detached()
{
// The fix introduces a list of (MonitoredItem, NotificationEventHandler) pairs onto
// RemoteSubscription so UnsubscribeAsync can `item.Notification -= handler` each one
// before deleting the subscription. We assert via reflection because the record is
// private — the public observable is just "no leaks" which is hard to assert
// synthetically.
var driverType = typeof(OpcUaClientDriver);
var remoteSubType = driverType.GetNestedType("RemoteSubscription", BindingFlags.NonPublic);
remoteSubType.ShouldNotBeNull("RemoteSubscription record should exist");
// The record must carry a property/field referencing the per-item handler delegates
// so detach is possible. Accept either a List of pairs or a parallel handler list —
// both are valid implementations; what matters is that the handler reference is
// reachable from the record.
var members = remoteSubType.GetMembers(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
var hasHandlerStorage = members.Any(m =>
m.Name.Contains("Handler", StringComparison.OrdinalIgnoreCase) ||
m.Name.Contains("Notif", StringComparison.OrdinalIgnoreCase) ||
m.Name.Contains("Item", StringComparison.OrdinalIgnoreCase));
hasHandlerStorage.ShouldBeTrue(
"RemoteSubscription must expose the per-item handler reference (or the MonitoredItem itself) " +
"so UnsubscribeAsync/ShutdownAsync can detach the Notification delegate before disposing the session.");
}
[Fact]
public void RemoteAlarmSubscription_record_carries_handler_delegate_so_it_can_be_detached()
{
var driverType = typeof(OpcUaClientDriver);
var remoteAlarmSubType = driverType.GetNestedType("RemoteAlarmSubscription", BindingFlags.NonPublic);
remoteAlarmSubType.ShouldNotBeNull("RemoteAlarmSubscription record should exist");
var members = remoteAlarmSubType.GetMembers(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
var hasHandlerStorage = members.Any(m =>
m.Name.Contains("Handler", StringComparison.OrdinalIgnoreCase) ||
m.Name.Contains("Notif", StringComparison.OrdinalIgnoreCase) ||
m.Name.Contains("EventItem", StringComparison.OrdinalIgnoreCase) ||
m.Name.Contains("Item", StringComparison.OrdinalIgnoreCase));
hasHandlerStorage.ShouldBeTrue(
"RemoteAlarmSubscription must expose the event-MonitoredItem (or its handler) reference " +
"so UnsubscribeAlarmsAsync/ShutdownAsync can detach the Notification delegate before " +
"disposing the session.");
}
[Fact]
public async Task UnsubscribeAsync_unknown_handle_does_not_throw_after_fix()
{
// Smoke: confirm the new detach logic doesn't break the existing unknown-handle
// no-op path (UnsubscribeAsync returns cleanly without an entry in _subscriptions).
using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-low-014");
await Should.NotThrowAsync(async () =>
await drv.UnsubscribeAsync(new FakeHandle(), TestContext.Current.CancellationToken));
}
[Fact]
public async Task UnsubscribeAlarmsAsync_unknown_handle_does_not_throw_after_fix()
{
using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-low-014-alarm");
await Should.NotThrowAsync(async () =>
await drv.UnsubscribeAlarmsAsync(new FakeAlarmHandle(), TestContext.Current.CancellationToken));
}
private sealed class FakeHandle : Core.Abstractions.ISubscriptionHandle
{
public string DiagnosticId => "fake-sub";
}
private sealed class FakeAlarmHandle : Core.Abstractions.IAlarmSubscriptionHandle
{
public string DiagnosticId => "fake-alarm-sub";
}
}