From 8e8199752f83d643193c47f243352f83c3386295 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 10:56:01 -0400 Subject: [PATCH] fix(server): resolve Medium code-review finding (Server-005) Add _nodeManagerDisposed field; set it under Lock in Dispose before detaching the alarm-service handler; check it in OnAlarmServiceTransition under the same Lock so an in-flight transition cannot dispatch to a ConditionSink whose DriverNodeManager is being concurrently disposed. Co-Authored-By: Claude Sonnet 4.6 --- code-reviews/Server/findings.md | 4 ++-- .../OpcUa/DriverNodeManager.cs | 21 ++++++++++++++++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/code-reviews/Server/findings.md b/code-reviews/Server/findings.md index e34e115..5062e0e 100644 --- a/code-reviews/Server/findings.md +++ b/code-reviews/Server/findings.md @@ -88,13 +88,13 @@ | Severity | Medium | | Category | Concurrency & thread safety | | Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Alarms/AlarmConditionService.cs:166`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:303-311` | -| Status | Open | +| Status | Resolved | **Description:** `OnValueChanged` raises `TransitionRaised` on the value-change thread; the subscriber `OnAlarmServiceTransition` drives `ConditionSink.OnTransition` → `alarm.ReportEvent`. `DriverNodeManager.Dispose` detaches the handler but does not synchronise against an in-flight `Invoke`. The service is process-shared across drivers, so a transition can dispatch to a `ConditionSink` whose `DriverNodeManager` is concurrently being disposed → `ReportEvent` on a torn-down node manager. **Recommendation:** Guard `OnAlarmServiceTransition` with a `_disposed` check under `Lock` before `sink.OnTransition`. Document that handlers must tolerate invocation during their owner's disposal. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `_nodeManagerDisposed` field; `Dispose(bool)` now sets it under `Lock` before detaching the handler; `OnAlarmServiceTransition` checks the flag under the same `Lock` and exits early, preventing forwarding to a sink after the node manager has begun disposal. ### Server-006 | Field | Value | diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs index 22189c5..56a5b69 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs @@ -110,6 +110,8 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder private readonly AlarmConditionService? _alarmService; private readonly Dictionary _conditionSinks = new(StringComparer.OrdinalIgnoreCase); private EventHandler? _alarmTransitionHandler; + // Guards OnAlarmServiceTransition against dispatch to a disposed node manager (Server-005). + private bool _nodeManagerDisposed; // Task #24 — Phase 7 Gap 1: route OPC UA Part 9 Acknowledge / Confirm method calls on // scripted alarm condition nodes to the ScriptedAlarmEngine so the engine state machine @@ -179,6 +181,11 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder ConditionSink? sink; lock (Lock) { + // Guard against a transition that arrives after Dispose has begun (Server-005). + // The alarm service is shared across drivers, so detaching the handler in Dispose + // does not synchronise against an in-flight Invoke — check _nodeManagerDisposed + // under the same lock Dispose sets it under so we never forward to a torn-down sink. + if (_nodeManagerDisposed) return; _conditionSinks.TryGetValue(t.ConditionId, out sink); } if (sink is null) return; @@ -302,10 +309,18 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder /// protected override void Dispose(bool disposing) { - if (disposing && _alarmService is not null && _alarmTransitionHandler is not null) + if (disposing) { - _alarmService.TransitionRaised -= _alarmTransitionHandler; - _alarmTransitionHandler = null; + // Mark disposed under Lock before detaching the handler so an in-flight + // OnAlarmServiceTransition that already holds the lock sees _nodeManagerDisposed + // and exits without forwarding to the sink (Server-005). + lock (Lock) { _nodeManagerDisposed = true; } + + if (_alarmService is not null && _alarmTransitionHandler is not null) + { + _alarmService.TransitionRaised -= _alarmTransitionHandler; + _alarmTransitionHandler = null; + } } base.Dispose(disposing); }