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

fix(server): resolve Medium code-review finding (Server-005)

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 10:56:01 -04:00
parent 2003b343bf
commit 8e8199752f
2 changed files with 20 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
code-reviews/Server/findings.md
View File

@@ -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 |

21
src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs
View File

@@ -110,6 +110,8 @@ public sealed class DriverNodeManager : CustomNodeManager2, IAddressSpaceBuilder
private readonly AlarmConditionService? _alarmService;
private readonly Dictionary<string, ConditionSink> _conditionSinks = new(StringComparer.OrdinalIgnoreCase);
private EventHandler<AlarmConditionTransition>? _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
/// </summary>
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);
}