From 42aa82de29e32ab68e23bbbee2d60c71e357401f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 08:17:55 -0400 Subject: [PATCH] fix(driver-opcuaclient): resolve Low code-review findings (Driver.OpcUaClient-011,014) - 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) --- code-reviews/Driver.OpcUaClient/findings.md | 18 +-- .../OpcUaClientDriver.cs | 94 +++++++++++- .../OpcUaClientLowFindingsRegressionTests.cs | 144 ++++++++++++++++++ 3 files changed, 239 insertions(+), 17 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLowFindingsRegressionTests.cs diff --git a/code-reviews/Driver.OpcUaClient/findings.md b/code-reviews/Driver.OpcUaClient/findings.md index fbce16b..9bd42d0 100644 --- a/code-reviews/Driver.OpcUaClient/findings.md +++ b/code-reviews/Driver.OpcUaClient/findings.md @@ -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 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index ee714db..846d4c2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -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(); 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 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); + /// + /// Live data-change subscription bookkeeping. Holds the SDK , + /// the local handle, and the per-MonitoredItem (item, handler) pairs so + /// / can detach the + /// Notification delegates before the SDK disposes the subscription + /// (Driver.OpcUaClient-014). + /// + private sealed record RemoteSubscription( + Subscription Subscription, + OpcUaSubscriptionHandle Handle, + IReadOnlyList ItemHandlers); + + /// + /// 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 OpcUaSubscriptionHandle, 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. + /// + 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); + /// + /// Live alarm-event subscription bookkeeping. Holds the SDK , + /// 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). + /// + private sealed record RemoteAlarmSubscription( + Subscription Subscription, + OpcUaAlarmSubscriptionHandle Handle, + MonitoredItem EventItem, + MonitoredItemNotificationEventHandler Handler); private sealed record OpcUaAlarmSubscriptionHandle(long Id) : IAlarmSubscriptionHandle { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLowFindingsRegressionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLowFindingsRegressionTests.cs new file mode 100644 index 0000000..298d1df --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLowFindingsRegressionTests.cs @@ -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; + +/// +/// Regression tests for the Low code-review findings cleared in the 2026-05-23 pass: +/// +/// Driver.OpcUaClient-011 — ValueRank comment + boundary semantics +/// 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 +/// +/// +[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"; + } +}