fix(opcua): condition events never block under the node-manager lock; folder-event Part 3 note (#838)
#838(a): each notifier's SDK MonitoredNode2 drains a bounded channel (4096, FullMode=Wait) and the synchronous NodeState.ReportEvent blocks when it is full. Condition events are reported under Lock, and the delivery task takes Lock on a verdict-cache miss (the SDK's own GetNodeMetadata, flag off too, and #826's IsNotifierEventVisibleTo). A burst of >4096 events under one Lock hold therefore deadlocked the node manager -- FolderEventFloodWireTests stalled at exactly 4097 events with enforcement on AND off (so it predates #826). ReportConditionEvent now goes through ReportEventWithoutBlocking: ReportEventAsync completes inline while channels have room (unchanged normal path); on overflow the report continues asynchronously holding no thread or lock, and later reports queue behind it so events keep report order. The deferred work holds only frozen snapshots and drains once Lock is released; client delivery stays bounded by each item's queue. #838(b): documented in docs/security.md that Part 3 ReceiveEvents applies to the event's EventType and SourceNode (the SDK check, kept first), not to the notifier; our decision is layered on top and non-condition events on a folder item fail closed under enforcement. The reflection item stays open.
This commit is contained in:
Joseph Doherty
@@ -204,6 +204,13 @@ Two things to know before relying on any of this:
|
||||
enforcement (`ServerEventRig.TagReferences()`); null ⇒ none (fail closed). Tests:
|
||||
`UnsFolderScopeWireTests.A_moved_alarm_stops_reaching_its_old_folders_subscribers…`,
|
||||
`NativeAlarmFolderEventAclWireTests`.
|
||||
⚠️ **Never report an event with the synchronous `NodeState.ReportEvent` while holding `Lock` (#838).** Each
|
||||
notifier's `MonitoredNode2` drains a BOUNDED channel (4096, `FullMode = Wait`) and its delivery task takes `Lock` on a
|
||||
verdict-cache miss (SDK `GetNodeMetadata` — flag off too — and #826's check); sync `ReportEvent` blocks on a full
|
||||
channel ⇒ a >4096 burst under one `Lock` hold deadlocked the node manager (stalled at 4097). Condition events go
|
||||
through `ReportEventWithoutBlocking` (async on overflow, ordered chain). Folder events follow Part 3's shape —
|
||||
`ReceiveEvents` on EventType + SourceNode (SDK check, kept first) AND our decision; non-condition events on a
|
||||
folder item fail closed under enforcement. Tests: `FolderEventFloodWireTests`.
|
||||
- **ns=0 standard nodes stay ungated by design, even with `Security:Acl:Enabled`** (#617) — `ServerStatus`,
|
||||
`NamespaceArray`, `ServiceLevel` etc. are served by the SDK's core node manager outside our authorizer and
|
||||
clients read them to connect and to judge redundancy health; they carry no plant data. Every raw/UNS node,
|
||||
|
||||
@@ -825,6 +825,41 @@ Behaviours worth knowing before enabling it:
|
||||
enforcement only while the installed generation carries that reference — a hand-built authority (tests) must
|
||||
supply `tagReferencingEquipment`. Inert with the flag off.
|
||||
|
||||
**Which permissions an event on a folder follows — checked against Part 3 (Gitea #838).** OPC UA Part 3's
|
||||
`RolePermissions` model gives events their own permission, `ReceiveEvents` (PermissionType bit 11), and the pinned
|
||||
SDK (2.0.1, `CustomNodeManager2.ValidateEventReceivePermissions`, documented there against Part 3 §8.55) applies it to
|
||||
**both the event's EventType and its SourceNode** — not to the notifier the item happens to sit on. The notifier's own
|
||||
permission only decides whether the item may be created/attached. So the spec's intent is *the folder AND the
|
||||
source*, never "an event reported through a folder inherits the folder's permissions". This server keeps that shape:
|
||||
the re-implemented `ValidateEventRolePermissions` runs the SDK's type + SourceNode check **first** and our decision
|
||||
only on top (both must pass). For the event kinds that reach folder items:
|
||||
- **Condition events** — SourceNode is the condition. Our decision is the condition rule above (the condition, or a
|
||||
folder it legitimately notifies through for the session), which in this ACL model is the deliberate
|
||||
generalisation of "the source": a `Tag` grant covers a native alarm's raw node and every UNS projection of it, and
|
||||
an `Equipment` grant covers the alarms it owns or references.
|
||||
- **A condition's Part 9 method audit events** (Acknowledge, Comment, Shelve…) — SourceNode is the condition, so they
|
||||
are resolved to it and follow exactly the condition rule (plus the SDK's audit/SignAndEncrypt guard on the
|
||||
Server-object path).
|
||||
- **Any other event on a folder item** (none is produced today; `GeneralModelChangeEvent` and the rejected-write
|
||||
`AuditWriteUpdateEvent` go to the Server object, #588) names no live condition and is **withheld** under
|
||||
enforcement — fail closed, which is stricter than the folder's permission and never laxer than Part 3.
|
||||
With the flag off only the SDK's own type + SourceNode `ReceiveEvents` check applies, as before.
|
||||
|
||||
**Event bursts never block under the node-manager lock (Gitea #838).** Each notifier with event items has an SDK
|
||||
`MonitoredNode2` whose delivery task drains a **bounded** channel (4096, `FullMode = Wait`), and the synchronous
|
||||
`NodeState.ReportEvent` **blocks** while that channel is full. Condition events are reported under the node
|
||||
manager's `Lock`, and the delivery task needs that same `Lock` on every per-event verdict-cache miss — in the SDK's
|
||||
own `GetNodeMetadata` (so **with the flag off too**, i.e. this predates #826) and in the #826 check. A burst of more
|
||||
than 4096 events under one hold of `Lock`, starting at a miss (a new item, or right after a deploy invalidated the
|
||||
verdicts), therefore deadlocked the node manager: `FolderEventFloodWireTests` stalled at exactly 4097 events, flag on
|
||||
and off. Condition events now go through `ReportEventWithoutBlocking`: `ReportEventAsync` completes inline while the
|
||||
channels have room (the normal case, unchanged); when one is full that report continues asynchronously, holding no
|
||||
thread and no lock, and later reports queue behind it so events still leave **in report order**. The deferred work
|
||||
holds only frozen snapshots — bounded by the burst — and drains once `Lock` is released; what a client receives is
|
||||
still bounded by its item's queue (oldest discarded). Pinned by `FolderEventFloodWireTests` (6000 events under one
|
||||
`Lock` hold with publishing disabled: completes, deferred reports drain to zero, the item delivers at most its
|
||||
revised `QueueSize` in increasing order ending with the last event, and the next event is delivered).
|
||||
|
||||
**Model-change and audit events on the Server object are gated too** (Gitea #588). The node manager's
|
||||
own server-wide events — `GeneralModelChangeEvent` (NodeAdded / NodeDeleted / DataTypeChanged) and the
|
||||
`AuditWriteUpdateEvent` of a rejected device write — used to go straight to `Server.ReportEvent`, where
|
||||
|
||||
@@ -1362,7 +1362,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3
|
||||
// subscriptions, per item, through the ACL (see OnConditionReportEvent).
|
||||
var snapshot = new InstanceStateSnapshot();
|
||||
snapshot.Initialize(SystemContext, alarm);
|
||||
alarm.ReportEvent(SystemContext, snapshot);
|
||||
ReportEventWithoutBlocking(alarm, snapshot);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
@@ -1375,6 +1375,95 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2, INodeManager3
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Serializes <see cref="_eventReportChain"/>.</summary>
|
||||
private readonly object _eventReportGate = new();
|
||||
|
||||
/// <summary>The tail of the deferred condition-event reports (Gitea #838); completed when none is pending.</summary>
|
||||
private Task _eventReportChain = Task.CompletedTask;
|
||||
|
||||
private int _deferredEventReports;
|
||||
|
||||
/// <summary>Condition-event reports currently deferred because a notifier's delivery channel was full (test observation).</summary>
|
||||
internal int DeferredEventReports => Volatile.Read(ref _deferredEventReports);
|
||||
|
||||
/// <summary>
|
||||
/// Reports a condition event through the condition's notifier chain WITHOUT blocking the caller, in report
|
||||
/// order (Gitea #838).
|
||||
/// </summary>
|
||||
/// <param name="source">The condition.</param>
|
||||
/// <param name="e">The frozen snapshot of the event.</param>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>Why not <c>NodeState.ReportEvent</c>.</b> Each notifier with event items has an SDK
|
||||
/// <c>MonitoredNode2</c> whose delivery task drains a BOUNDED channel (4096, <c>FullMode = Wait</c>), and
|
||||
/// the synchronous <c>ReportEvent</c> BLOCKS when that channel is full. Condition events are reported
|
||||
/// under <c>Lock</c>, and the delivery task takes <c>Lock</c> on every per-event verdict-cache miss (the
|
||||
/// SDK's own <c>GetNodeMetadata</c>, and #826's <see cref="IsNotifierEventVisibleTo"/>). So a burst of more
|
||||
/// than 4096 events under one hold of <c>Lock</c>, starting at a miss, parked the consumer on <c>Lock</c>
|
||||
/// while the producer waited on the consumer: a deadlock that froze the whole node manager (pinned by
|
||||
/// <c>FolderEventFloodWireTests</c>, which stalled at exactly 4097 events before this).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>What this does.</b> The common case is unchanged and synchronous: <c>ReportEventAsync</c> completes
|
||||
/// inline while every channel has room. Only when a channel is full does that report continue
|
||||
/// asynchronously (awaiting channel space, holding no thread and no lock), and every later report queues
|
||||
/// behind it, so events still leave in the order they were reported. The deferred work holds only frozen
|
||||
/// snapshots, so it is bounded by the burst that caused it and drains once <c>Lock</c> is released.
|
||||
/// Delivery to clients stays bounded by each monitored item's own queue (oldest discarded). A deferred
|
||||
/// report runs the condition's synchronous <c>OnReportEvent</c> hook (<see cref="OnConditionReportEvent"/>)
|
||||
/// off <c>Lock</c>; that hook takes <c>Lock</c> itself where it reads guarded state.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private void ReportEventWithoutBlocking(NodeState source, IFilterTarget e)
|
||||
{
|
||||
lock (_eventReportGate)
|
||||
{
|
||||
if (_eventReportChain.IsCompleted)
|
||||
{
|
||||
var report = source.ReportEventAsync(SystemContext, e);
|
||||
if (report.IsCompletedSuccessfully) return;
|
||||
Interlocked.Increment(ref _deferredEventReports);
|
||||
_eventReportChain = ObserveEventReportAsync(report.AsTask(), source);
|
||||
return;
|
||||
}
|
||||
|
||||
Interlocked.Increment(ref _deferredEventReports);
|
||||
_eventReportChain = ReportAfterAsync(_eventReportChain, source, e);
|
||||
}
|
||||
}
|
||||
|
||||
private async Task ReportAfterAsync(Task previous, NodeState source, IFilterTarget e)
|
||||
{
|
||||
await previous.ConfigureAwait(false); // never faults: every link observes its own failure
|
||||
Task report;
|
||||
try
|
||||
{
|
||||
report = source.ReportEventAsync(SystemContext, e).AsTask();
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
report = Task.FromException(ex);
|
||||
}
|
||||
|
||||
await ObserveEventReportAsync(report, source).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
private async Task ObserveEventReportAsync(Task report, NodeState source)
|
||||
{
|
||||
try
|
||||
{
|
||||
await report.ConfigureAwait(false);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
_logger.LogError(ex, "OtOpcUaNodeManager: failed to report Part 9 condition event for {NodeId}", source.NodeId);
|
||||
}
|
||||
finally
|
||||
{
|
||||
Interlocked.Decrement(ref _deferredEventReports);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The gate-relevant slice of a Part 9 condition's state — exactly the fields that drive a
|
||||
/// condition event AND that an <see cref="AlarmConditionSnapshot"/> can change. As a record, two
|
||||
|
||||
+136
@@ -0,0 +1,136 @@
|
||||
using Opc.Ua;
|
||||
using Opc.Ua.Client;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
|
||||
using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
|
||||
using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Authorization;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests;
|
||||
|
||||
/// <summary>
|
||||
/// Gitea #838 (follow-up to #826): the per-folder event ACL runs on the SDK's <c>MonitoredNode2</c> delivery task,
|
||||
/// and needs the node manager's <c>Lock</c> on a verdict-cache miss. That delivery task drains a BOUNDED channel
|
||||
/// (4096, <c>FullMode = Wait</c>), and the synchronous <c>NodeState.ReportEvent</c> the node manager calls under
|
||||
/// <c>Lock</c> BLOCKS when the channel is full. So a burst of more than 4096 events reported under one hold of
|
||||
/// <c>Lock</c>, starting at a verdict-cache miss, parks the consumer on <c>Lock</c> while the producer waits on the
|
||||
/// consumer — a deadlock that also freezes every other operation needing <c>Lock</c>. Publishing is switched off
|
||||
/// on the client so nothing downstream drains either (the slow / absent consumer case).
|
||||
/// </summary>
|
||||
public sealed class FolderEventFloodWireTests
|
||||
{
|
||||
private const string EqA = ServerEventRig.EqA;
|
||||
private const int FloodSize = 6000; // > the SDK's 4096-slot per-node channel
|
||||
|
||||
/// <param name="enforced">
|
||||
/// Per-folder ACL on or off. The hazard does NOT need #826: with enforcement off the SDK's own
|
||||
/// <c>ValidateEventRolePermissions</c> still reaches <c>GetNodeMetadata</c>, which takes <c>Lock</c>, on the
|
||||
/// first event to a new item.
|
||||
/// </param>
|
||||
[Theory]
|
||||
[InlineData(true)]
|
||||
[InlineData(false)]
|
||||
public async Task A_flood_beyond_the_channel_under_the_node_manager_lock_neither_deadlocks_nor_grows_unbounded(bool enforced)
|
||||
{
|
||||
var ct = TestContext.Current.CancellationToken;
|
||||
var rig = await ServerEventRig.StartAsync("Folder838Flood", encrypted: false, ct);
|
||||
var deadlocked = false;
|
||||
ISession? keyed = null;
|
||||
var reported = 0;
|
||||
try
|
||||
{
|
||||
rig.KeyRoles = [];
|
||||
rig.MaterialiseNative();
|
||||
NodeAcl[] grants = [KeyEquipmentGrant(EqA)];
|
||||
rig.Enforce(enforced, [ServerEventRig.Key()], grants);
|
||||
|
||||
keyed = await rig.ConnectAsync(anonymous: false, ct);
|
||||
var events = new RecordedEvents();
|
||||
var subscription = await RecordedEvents.SubscribeAsync(keyed, events, ct);
|
||||
var onA = RecordedEvents.AddEventItem(subscription, ServerEventRig.UnsNodeId(keyed, EqA));
|
||||
await subscription.ApplyChangesAsync(ct);
|
||||
onA.Status.Error.ShouldBeNull();
|
||||
|
||||
// No publishing: the server-side item queue (QueueSize 1000) is the only sink, as with a stalled client.
|
||||
await subscription.SetPublishingModeAsync(false, ct);
|
||||
|
||||
// The whole burst under ONE hold of the node manager's Lock — the shape of any multi-event apply/projection
|
||||
// pass — starting from an uncached verdict (the item is new), so the delivery task needs Lock for event 1.
|
||||
var flood = Task.Factory.StartNew(() =>
|
||||
{
|
||||
lock (rig.Nm.Lock)
|
||||
{
|
||||
for (var i = 0; i < FloodSize; i++)
|
||||
{
|
||||
rig.Project(native: true, State($"f{i}"));
|
||||
Volatile.Write(ref reported, i + 1);
|
||||
}
|
||||
}
|
||||
}, ct, TaskCreationOptions.LongRunning, TaskScheduler.Default);
|
||||
|
||||
try
|
||||
{
|
||||
await flood.WaitAsync(TimeSpan.FromSeconds(60), ct);
|
||||
}
|
||||
catch (TimeoutException)
|
||||
{
|
||||
deadlocked = true;
|
||||
throw new ShouldAssertException(
|
||||
$"reporting {FloodSize} folder events under the node manager's Lock stalled after "
|
||||
+ $"{Volatile.Read(ref reported)} — the SDK's per-node event channel filled while its consumer waited "
|
||||
+ "on Lock (deadlock)");
|
||||
}
|
||||
|
||||
// The overflow was deferred rather than blocking, and drains once Lock is free.
|
||||
await RecordedEvents.WaitUntilAsync(() => rig.Nm.DeferredEventReports == 0);
|
||||
// …and the SDK's channel (up to 4096 accepted events) finishes draining into the item queue, still with
|
||||
// publishing off, so the count below measures the item queue and not events in transit.
|
||||
await Task.Delay(TimeSpan.FromSeconds(2), ct);
|
||||
|
||||
// Delivery is bounded by the item's queue (QueueSize 1000, oldest discarded), not by the flood, and the
|
||||
// events that survive arrive in report order, ending with the last one reported.
|
||||
await subscription.SetPublishingModeAsync(true, ct);
|
||||
var last = $"f{FloodSize - 1}";
|
||||
await RecordedEvents.WaitUntilAsync(() => events.Has(onA, last));
|
||||
await Task.Delay(TimeSpan.FromMilliseconds(500), ct);
|
||||
var flooded = events.Of(onA)
|
||||
.Select(RecordedEvents.Message)
|
||||
.Where(m => m?.StartsWith('f') == true)
|
||||
.Select(m => int.Parse(m![1..], System.Globalization.CultureInfo.InvariantCulture))
|
||||
.ToList();
|
||||
flooded.Count.ShouldBeGreaterThan(0);
|
||||
flooded.Count.ShouldBeLessThanOrEqualTo((int)onA.Status.QueueSize, "delivery is bounded by the item's queue, not the flood");
|
||||
flooded.ShouldBe(flooded.Order().ToList(), "events leave in the order they were reported");
|
||||
flooded[^1].ShouldBe(FloodSize - 1);
|
||||
|
||||
// And the node manager is live: a later event is decided by the per-folder ACL and delivered.
|
||||
rig.Project(native: true, State("after"));
|
||||
await RecordedEvents.WaitUntilAsync(() => events.Has(onA, "after"));
|
||||
}
|
||||
finally
|
||||
{
|
||||
// A deadlocked rig cannot be disposed (its Lock is held by a blocked thread) and neither can a session on it
|
||||
// (CloseSession needs the server); leak both rather than hang the run.
|
||||
if (!deadlocked)
|
||||
{
|
||||
keyed?.Dispose();
|
||||
await rig.DisposeAsync();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static AlarmConditionSnapshot State(string message) =>
|
||||
new(true, false, false, true, AlarmShelvingKind.Unshelved, 700, message);
|
||||
|
||||
private static NodeAcl KeyEquipmentGrant(string equipment) => new()
|
||||
{
|
||||
NodeAclId = $"acl-key-{equipment}",
|
||||
ClusterId = ServerEventRig.Cluster,
|
||||
Principal = ServerEventRig.KeyPrincipal,
|
||||
ScopeKind = NodeAclScopeKind.Equipment,
|
||||
ScopeId = $"eq-{equipment}",
|
||||
PermissionFlags = NodePermissions.ReadOnly,
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user