Files
UA-.NETStandard/plans/NodeManagerAnalysis.md
Marc Schier e05a820c91 Removal of the exposed diagnostics and node locks (#4188)
## What this is

Design analysis for `Opc.Ua.Server`, the two rounds of design
exploration behind it, and the first two implementation chunks it
recommends.

Draft: the analyses are ready to read, but the *conclusions* deserve
discussion before the larger phases proceed. Later chunks stack onto
this PR as they land.

| File | Kind | Lines |
|---|---|---:|
| `plans/NodeManagerAnalysis.md` | Evidence — the node-manager plugin
surface | 488 |
| `plans/ServerRuntimeAnalysis.md` | Evidence — the server runtime
cluster | 576 |
| `plans/ServerDesignExploration.md` | Design round 2 — delivery
strategy | 1,394 |
| `plans/ServerDesignExplorationAlt.md` | Design round 1 — four
interface designs | 2,119 |
| `src/Opc.Ua.Types/State/NodeState.cs` | **Code** — node self-guards
attributes | |
| `src/Opc.Ua.Types/State/BaseVariableState.cs` | **Code** — value
accessors take the lock | |
| `src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs` | **Code** —
6 external locks removed | |
| `tests/…/NodeStateAttributeConcurrencyTests.cs` | **Code** —
concurrency safety net | 449 |

`docs/` is untouched — it holds shipped feature documentation, and none
of this is shipped API.

## The analyses

Counted mechanically over `src`, `samples` and `tests`; **every figure
is reproducible** from commands printed in each document.

**Node manager surface.** `AsyncCustomNodeManager` declares **93
`virtual`/`abstract` members** (80 distinct names). Across the
repository: **112 override sites, 25 distinct names ever overridden, 55
of 80 never**. Six carry nearly all real use. For scale,
`ReferenceNodeManager` is 6,373 lines and overrides **8**.

**Server runtime.** Seven findings, **two of which recommend no
change**. Findings 1–5 are filed as #4183–#4187. The headline is an
`object`-typed sync root published on three public interfaces and taken
in **88 `lock` statements across 7 files**, including a shipped sample.

**What is deliberately not flagged.** `IStandardServer` — 9 members over
5,575 lines, 16 of 37 virtuals used — is identified as the reference
shape to preserve. `IApplicationInstance` and the already-`[Obsolete]`
`Stop()` are examined and left alone.

## The design exploration

**Round 1** explored four alternative interfaces in parallel under the
current back-compat constraint. Its value is the convergence: four
designers who never saw each other's work independently arrived at a 6–9
member `IServerContext` replacing the 57-member locator, removal of the
`object` node-manager handle, elimination of the `Processed` protocol,
and `IHistorianProvider` as the pattern to copy.

**Round 2** re-runs it with back-compat lifted and asks how to deliver:
full rewrite versus side-by-side fork. **Both option designers
independently declined to recommend their own assignment.** Three
structural facts were verified:

1. **The fork line already sits at `IAsyncNodeManager`** — old and new
node managers can coexist with zero runtime changes, so a fork
duplicates ~10% of the assembly, not all of it.
2. **`[OpcUaShim]` cannot shim an inheritance surface** — honest
auto-fix ceiling for the authoring surface is **~10%**, against ~70% for
the shipped analyzers.
3. **A second lock leak exists** — `NodeState` takes `lock (this)` under
a `CA2002` suppression whose own comment documents the contract.

Recommendation: **neither pure option**, but a staged path reaching the
rewrite end state incrementally, gated on migrating the five
framework-internal node managers that today make it impossible for the
old base to ever freeze.

## Implementation: NodeState now guards its own attributes

This removes the prose-only contract in which `NodeState` took
`lock(this)` and every external caller was required to take
`lock(source)` on the node instance. The code already carried a TODO
describing exactly this fix and noting it required updating every
external site.

- The node self-guards rather than relying on callers.
`BaseVariableState` gains a **fully private** `System.Threading.Lock`
covering its value fields; `NodeState`'s other collections keep their
existing private locks.
- All **6** `lock(this)` sites and both `CA2002`/`RCS1059` suppressions
are gone.
- `Value`, `Timestamp` and `StatusCode` on `BaseVariableState` take the
lock too — without that, a direct property write raced an attribute read
once the external lock was gone.
- **6 of 9** external sites removed. Five were already redundant
(`GetReferences`, `GetChildren`, `ReferenceExists` and
`SetAreEventsMonitored` guard themselves; `FindMethod` reaches its state
via `GetChildren`); the sixth was an attribute read now covered by the
node's own lock.
- Attribute read/write deliberately take **no** node-wide lock: both
dispatch to caller-supplied handlers (`OnReadValue` and siblings) that
may take locks of their own, and holding a lock on the node across one
gives the node no fixed place in the lock order.
- **3 deliberately retained and annotated in code**: two `CreateBrowser`
calls, which are not internally synchronized (tracked in #4216), and one
in `CustomNodeManager`, whose class is slated for deletion so
refactoring its lock would be wasted work.

### No lock is exposed anywhere, to anyone

An earlier revision put the attribute lock on `NodeState` as `private
protected`, which still published a lock-sharing contract on the
inheritance surface. Review pushed back and it is gone:
`BaseVariableState` was the only user, so the lock moved there as a
plain `private readonly Lock`. There is no member for a derived type to
call, in this assembly or any other.

`BaseVariableValue` was the last public leak — it handed out the lock
guarding its value through a `public Lock` property, so any caller could
hold it across a call back into the stack. That property is removed:

- Derived value classes (what the source generator emits for every
structure variable) synchronize through a `protected EnterLock()` /
`ExitLock()` pair. Callback helpers were the first choice, but the
generated bodies pass `ref Variant` / `ref StatusCode` / `ref
DateTimeUtc`, and `ref` arguments cannot be captured by a lambda. The
lock *object* stays private either way.
- `NodeStateTemplates` emits the new form.
- `ServerInternalData`, the one cross-assembly caller, was reading the
lock back off a value it had itself constructed with that lock. It now
takes `m_diagnosticsLock` directly.

`NodeBrowser.DataLock` remains `protected` and is tracked in #4215: a
subclass extending `Next()` legitimately needs its enumeration state
atomic with the base browser's, so the fix needs a base-owned
acquisition rather than a deletion.

### Migration coverage for the removed lock members

None of the removed lock members get an `[Obsolete]` shim, and the
migration guide now says why: a shim would have to be `object`-typed to
keep old call sites compiling, and on .NET 9+ `lock (o)` on a
statically-`object` reference is `Monitor.Enter`, which does **not**
interlock with `Lock.Enter()`. It would compile, run, and silently
synchronize nothing — strictly worse than a compile error. Re-adding a
member to a downstream-implemented interface is also a break in its own
right on `net48` / `netstandard2.0`, where there are no default
interface methods.

Instead, every removed member has a named replacement on the migration
path:

| Rule | Covers | Replacement it names |
|---|---|---|
| `UA0024` | `IServerInternal` / `ISession` / `ISubscription`
`DiagnosticsLock`, `DiagnosticsWriteLock` |
`UpdateServerDiagnostics(...)` / `UpdateDiagnostics(...)`,
`ReadServerDiagnostics` / `ReadDiagnostics` |
| `UA0025` | `ILocalNode.DataLock`, `Node.DataLock` | a lock the caller
owns; the node guards its own state |
| `UA0026` | `BaseVariableValue.Lock` | a lock passed to the
`BaseVariableValue` constructor |

Each rule fires on sources still written against 1.5.378, where the
compiler only says a member is missing. `docs/MigrationGuide.md` gains a
section per removal with before/after.

### The tests earned their keep

The four concurrency tests were landed **first**, against the unmodified
code, because removing a synchronization contract cannot be verified by
the compiler and the existing `NodeStateHandlerConcurrencyTests` covers
a different concern (racing a handler assignment against a write, not
reader/writer exclusion). They now run with **no external locking**,
which is what demonstrates the node guards itself.

They caught two real problems, and one of them was mine:

- The `Value`/`StatusCode`/`Timestamp` setters bypassed the new lock — a
genuine race.
- One test asserted an invariant the design does not offer. Two property
setters are two lock acquisitions, so that pair was **never** atomic; it
only passed originally because the test held a lock across both. It now
uses a single attribute write carrying a whole `DataValue`, which
genuinely is atomic and is the path the server uses.

A third issue was found and fixed before it shipped: the reader loops
gated on `!writer.IsCompleted`, so a reader scheduled after the writer
finished performed zero reads — failing two runs in three on `net48`.
Converted to `do…while`; now five consecutive green runs.

**Verified:** `Stack.State` 116 passed on `net10.0` *and* `net48`; the
concurrency category green 5/5 consecutive on `net48`;
`Opc.Ua.Types.Tests` 8,456 passed; the server node-manager suite 1,205
passed, including `NodeManagerLifecycleTests` and
`AsyncCustomNodeManagerTests` which exercise the changed paths.

## Two corrections to my own published figures

Both were found while starting implementation, and both are recorded on
#4183.

**`ForceDiagnosticsScan()` was overstated.** The analysis claimed the
`DiagnosticsWriteLock` getter *"triggers a full address-space
diagnostics rescan"*. It is in fact a single flag assignment
(`DiagnosticsNodeManager.cs:700-703`) consumed by the read paths and the
periodic timer. The defect stands — a side-effecting property getter
with a racy write on a synchronisation path — but its cost was
misstated.

**The contract was 15 sites, not 19.** A naive search matched ten
comment and `#pragma` lines that *mention* `lock(source)` while
explaining it. The documents now carry a counting command that excludes
comment matches.

A third finding expanded scope the other way: the `object`-typed lock
leak is **repo-wide**, with ten such members across `Opc.Ua.Core`,
`Opc.Ua.Types` and `Opc.Ua.Server` — including `Node.DataLock => this`
and `ApplicationConfiguration.PropertiesLock => m_properties`, the same
defect class as `Subscription.DiagnosticsLock`.

## Caveats stated in the documents

- **Nothing in the designs is implemented or benchmarked.** Every
performance claim across all seven designs is a design intention, not a
measurement.
- The larger phases are a **major undertaking against a live plugin
API**; the documents say so rather than letting the neatness of the
proposals imply otherwise.
- The recommended sequencing puts low-risk, self-contained steps first
and notes they are **worth doing regardless** of which strategy is
eventually chosen.
- Round 2 notes the two option designers' cost estimates are **not
independent of their assignments**.

## Review guidance

The two analyses make factual claims checkable by running the included
commands. The design explorations are exploratory and need not be agreed
with to merge. The code change is small, test-backed, and independent of
which delivery strategy is eventually chosen.

Reasonable outcomes include merging as-is, merging only the analyses and
the lock fix, or disagreeing with the recommendation while keeping the
evidence.

---

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63
2026-08-10 06:47:50 +02:00

24 KiB
Raw Permalink Blame History

Node Manager Seam Analysis

Status: design analysis / proposal — not shipped API. This document records an evidence-based assessment of the node-manager authoring interface in Opc.Ua.Server. It describes what the surface looks like today and proposes a narrower shape. Nothing here has been implemented. For the current, shipped node-manager documentation see NodeManagers.md.

Analysis performed against commit e73e71184 on master.

Table of contents

Purpose

Opc.Ua.Server is roughly 145,000 lines. Most of it is organised into modules with small interfaces and large implementations. The node-manager authoring surface is not: it presents 93 extension points to buy roughly 5 that callers use.

This document quantifies that gap so the decision to re-cut the seam can be made on evidence rather than impression. It deliberately does not propose moving implementation code. The implementation is good and earns its keep; only the interface is mis-shaped.

Vocabulary

The analysis uses deep-module design terms consistently:

Term Meaning here
Module Anything with an interface and an implementation — a class, an interface, a package.
Interface Everything a caller must know to use a module correctly: the type signature plus invariants, ordering constraints, error modes, and required cooperation protocols.
Implementation The body of code inside the module.
Depth Leverage at the interface — how much behaviour a caller gets per unit of interface they must learn.
Seam The place where a module's interface lives; where behaviour can be altered without editing in that place.
Adapter A concrete thing satisfying an interface at a seam.

Two rules are applied throughout:

  • The deletion test. Imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep.
  • One adapter means a hypothetical seam. Two adapters means a real one.

Method

Evidence was gathered mechanically over src, samples, and tests, excluding obj and bin:

  1. Every public/protected virtual/abstract declaration in AsyncCustomNodeManager.cs was enumerated.
  2. Every override site in any class deriving from AsyncCustomNodeManager, FluentNodeManagerBase, or CoreNodeManager was counted and attributed to one of four consumer categories. The scan is textual, so its output is reviewed for synchronous-twin members and regex false positives before use.
  3. Each virtual member was cross-referenced against the interface declarations in INodeManager.cs to separate interface implementations from pure extension hooks.

Consumer categories:

Category Meaning
core Inside src/Opc.Ua.Server — framework-internal, an internal seam
sib Other src libraries (GDS, PubSub, WotCon, XRegistry, Positioning) — first-party callers
sample samples/ — representative of external authors
test tests/ — may indicate testing past the interface

Commands are listed under Reproducing the evidence.

Finding 1: the assembly is bimodal

Opc.Ua.Server already contains several genuinely deep modules. These are the reference shape and should be left alone:

Module Interface size Behind the seam
IHistorianProvider 2 members + narrow capability interfaces ~5,900 lines; HistorianDispatcher (2,370 lines) routes
ILocalAddressSpace 7 members, invariants documented in XML docs node graph; two real adapters (PredefinedNodes, dictionary-backed test double)
IMonitoredItemManager 7 members sampling groups, queues, queue handlers
IFileSystemProvider 11 members 4,145 lines

IHistorianProvider is worth reading as the model: an umbrella of two members (IsHistorizingAsync, GetCapabilitiesAsync) with providers opting into Part 11 features through narrow capability interfaces (IHistorianDataProvider, IHistorianAtTimeProvider, IHistorianEventProvider, and so on). See HistoricalAccess.md.

Finding 2: the node-manager seam is the outlier

Interface inventory

NodeManager/INodeManager.cs is 1,158 lines and declares 22 public interfaces:

Interface Members Notes
INodeManager 23 Mandatory core
INodeManager2 +3 Version-suffixed extension
INodeManager3 +4 Version-suffixed extension
IAsyncNodeManager ~20 Parallel async family
INodeManagerFactory / IAsyncNodeManagerFactory 2 each Construction
15 × I*AsyncNodeManager 1–5 each Opt-in capability interfaces — the correct pattern

The seam has been widened twice by version suffix (2, 3) rather than re-cut.

Base-class surface

Type Lines public protected virtual/abstract
AsyncCustomNodeManager 8,028 59 92 93
CustomNodeManager2 6,302 52 86 93
MasterNodeManager 7,601 86 21 39

The 93 virtual/abstract declarations resolve to 80 distinct names (the remainder are overloads). That figure — 93 extension points — is the effective interface presented to the 33 classes deriving from these bases.

What callers actually use

Across src, samples, and tests there are 112 override sites. The raw scan reports 30 distinct names; five are discounted — LoadPredefinedNodes, OnSubscribeToEvents, and OnNodeRemoved are the synchronous twins declared on CustomNodeManager2 rather than AsyncCustomNodeManager, and if plus CreateSessionServerSignature are regex false positives. That leaves 25 distinct names of the 80. 55 of the 80 names are never overridden anywhere.

Full override census, all consumer categories:

Member Total core sib sample test
CreateAddressSpaceAsync 21 5 9 5 2
Dispose 14 4 5 4 1
New 12 3 5 3 1
LoadPredefinedNodesAsync 11 1 8 1 1
DeleteAddressSpaceAsync 8 2 4 1 1
AddBehaviourToPredefinedNodeAsync 5 1 3 1 0
OnMonitoredItemCreated 4 2 1 1 0
OnMonitoredItemDeletedAsync 3 1 1 1 0
GetHistorianProvider 3 0 0 2 1
GetManagerHandleAsync 3 1 1 1 0
ValidateNodeAsync 3 1 1 1 0
OnMonitoringModeChangedAsync 3 1 1 1 0
OnSubscribeToEventsAsync 2 1 0 0 1
ConditionRefreshAsync 2 0 0 1 1
OnMonitoredItemModifiedAsync 2 0 1 1 0
OnNodeRemovedAsync 2 1 0 0 1
SubscribeToAllEventsAsync 1 0 0 0 1
SessionClosingAsync 1 1 0 0 0
SessionActivatedAsync 1 0 0 0 1
ReadAsync 1 0 0 0 1
AddPredefinedNodeAsync 1 1 0 0 0
AddReferencesAsync 1 1 0 0 0
DeleteReferenceAsync 1 1 0 0 0
CallAsync 1 0 0 1 0
ValidateViewDescription 1 1 0 0 0

Representative consumers:

Consumer Lines Overrides
ReferenceNodeManager (sample) 6,373 8
FluentNodeManagerBase 280 4

Six members account for the overwhelming majority of all override sites: CreateAddressSpaceAsync, Dispose, New, LoadPredefinedNodesAsync, DeleteAddressSpaceAsync, AddBehaviourToPredefinedNodeAsync.

This is the depth deficit. The implementation is genuinely deep — roughly 22,000 lines of browse, read, write, call, history, and monitored-item machinery across the three base classes. The interface charges an author 93 extension points to buy the handful they need. Leverage per unit of interface learned is poor.

Interface complexity outside the type signature

Under the definition used here, the interface includes every fact a caller must know. Several load-bearing contracts on this seam are enforced only by prose:

  • Opaque handles. object? GetManagerHandle(NodeId) returns an untyped handle the caller must round-trip correctly. This also conflicts with the repository rule against object in public API.
  • The Processed flag protocol. "The node manager must ignore ReadValueId with the Processed flag set to true. The node manager must set the Processed flag for any ReadValueId that it processes." This cooperative-multiplexing contract is restated six times in INodeManager.cs and is not expressible in the type system.
  • Index-aligned accumulators. Read, Write, Call, HistoryRead and friends take parallel IList<T> value and error collections that the caller pre-sizes and the implementer must index-align.
  • Pre-populated outputs. Browse takes ref ContinuationPoint and documents that "the references parameter may already contain references when the method is called", which the implementer must account for when deciding whether to return a continuation point.

Finding 3: the interface seam and the real seam are in different places

  • Across the entire repository there is exactly one direct implementation of INodeManager outside the framework: SampleNodeManager in samples/Quickstarts.Servers. By the one-adapter rule, the INodeManager seam is hypothetical. All 33 real node managers derive from the base classes instead.
  • IAsyncNodeManager exposes SyncNodeManager : INodeManager — the async interface publishes the sync adapter through itself, letting callers cross back over the seam.
  • The two bridging adapters (AsyncNodeManagerAdapter, 915 lines; SyncNodeManagerAdapter) span a technology duality (sync versus async), not a domain one.
  • Internally, NodeManagerRoutingTable is IReadOnlyList<IAsyncNodeManager>. Everything is already normalised to the async interface before dispatch.

The deletion test

Module Delete it Verdict
AsyncCustomNodeManager Complexity reappears in all 33 subclasses Earns its keep. The implementation is not the problem.
INodeManager / 2 / 3 Little reappears — routing already runs on IAsyncNodeManager Compatibility surface, not a load-bearing seam.
The 11 History*Async virtuals Nothing reappears — they are handle loops delegating into HistorianDispatcher.Dispatch*Async Pure pass-through.
MasterNodeManager Complexity reappears, but much of it is caused by the seam below it (fan-out, Processed multiplexing, handle resolution) Shrinks if the seam narrows; do not attack directly.

Full classification of the 93 virtual members

The 80 distinct names partition exactly into five buckets, verified with no gaps, duplicates, or phantoms.

Bucket Count Action
K — Keep at seam 7 The authoring interface
O — Observer hooks 9 Collapse into one opt-in interface
C — Capability interface 26 Remove virtual; keep the existing opt-in interfaces
D — Convert to DI 2 Replace inheritance-based provider selection with injection
H — Hide 36 Make private or sealed
Total 80

K — Keep at seam (7)

The members that actually carry authoring weight.

Member Overrides Note
CreateAddressSpaceAsync 21 Primary authoring entry point
Dispose 14 IDisposable; comes free, not part of the node-manager seam
New 12 NodeId factory (INodeIdFactory)
LoadPredefinedNodesAsync 11 NodeSet loading
DeleteAddressSpaceAsync 8 Teardown
AddBehaviourToPredefinedNodeAsync 5 Attach behaviour to loaded nodes
NamespaceUris 0 Set through the constructor; should be non-virtual

Discounting Dispose (inherited from IDisposable) and NamespaceUris (constructor state), the genuine authoring seam is 5 members.

O — Observer hooks (9)

Every member here answers "tell me when X happened". Nine separate virtuals buy one concept.

Member Overrides
OnMonitoredItemCreated 4
OnMonitoredItemDeletedAsync 3
OnMonitoringModeChangedAsync 3
ValidateNodeAsync 3
OnMonitoredItemModifiedAsync 2
OnSubscribeToEventsAsync 2
OnNodeRemovedAsync 2
SessionClosingAsync 1
SessionActivatedAsync 1

Proposal: one opt-in INodeManagerObserver, resolved through DI, keeping the mandatory seam at 5 members.

C — Capability interface (26)

Every member in this bucket is already declared on an I*AsyncNodeManager capability interface. The base class then re-exposes each one as virtual, so an author meets the same decision twice — once as "implement this interface", once as "override this method". Only 7 of the 26 are ever overridden, mostly once.

ReadAsync · WriteAsync · CallAsync · TranslateBrowsePathAsync · HistoryReadAsync · HistoryUpdateAsync · ConditionRefreshAsync · SetMonitoringModeAsync · CreateMonitoredItemsAsync · ModifyMonitoredItemsAsync · DeleteMonitoredItemsAsync · TransferMonitoredItemsAsync · RestoreMonitoredItemsAsync · AddNodeAsync · DeleteNodeAsync · AddReferenceAsync · AddReferencesAsync · DeleteReferenceAsync · AllowNodeManagement · GetManagerHandleAsync · SubscribeToEventsAsync · SubscribeToAllEventsAsync · ValidateRolePermissionsAsync · IsNodeInView · IsNodeInViewAsync · FindMethodStateAsync

Proposal: drop the virtual modifier. The capability interfaces already provide the opt-in route and are documented in NodeManagement.md and AsyncServerSupport.md.

D — Convert to dependency injection (2)

Member Overrides Note
GetHistorianProvider 3 (2 sample, 1 test) Returns null by default; composed as HistorianDispatcher.ResolveProvider(Server, node, GetHistorianProvider(node))
HasHistorianProvider 0 Derived from the above

This is provider selection by inheritance. Samples override it because there is no injection point at that layer. The repository convention is a provider model with injectable providers, so this should become an injected resolver.

H — Hide (36)

34 of these have zero overrides. Two (AddPredefinedNodeAsync, ValidateViewDescription) are overridden only inside Opc.Ua.Server itself — internal seams that should not surface at the authoring interface.

History fan-out (11) — verified to be handle loops delegating straight into HistorianDispatcher.Dispatch*Async, with the real behaviour behind the 2-member IHistorianProvider. Roughly 1,000 lines of pass-through published as 11 extension points that nobody uses.

HistoryReadRawModifiedAsync · HistoryReadAtTimeAsync · HistoryReadProcessedAsync · HistoryReadEventsAsync · HistoryUpdateDataAsync · HistoryUpdateEventsAsync · HistoryUpdateStructureDataAsync · HistoryDeleteRawModifiedAsync · HistoryDeleteAtTimeAsync · HistoryDeleteEventsAsync · HistoryReleaseContinuationPointsAsync

Monitored-item internals (13) — IMonitoredItemManager (7 members) is already the correct seam for these.

CreateMonitoredItemAsync · ModifyMonitoredItemAsync · DeleteMonitoredItemAsync · RestoreMonitoredItem · ReadInitialValue · ValidateMonitoringFilterAsync · ReviseAggregateFilterAsync · ValidationComplete · OnCreateMonitoredItemsComplete · OnDeleteMonitoredItemsCompleteAsync · OnModifyMonitoredItemsCompleteAsync · OnSetMonitoringModeCompleteAsync · OnMonitoredItemsTransferredAsync

Note the smell inside this group: the batch On*Complete family has zero overrides while the singular On*Created / On*Deleted / On*Modified family (bucket O) has nine. Two parallel hook families exist; only one is used.

Namespace and view predicates (5)

IsHandleInNamespace · IsNodeIdInNamespace · IsReferenceInView · FindNodeInCacheAsync · ValidateViewDescription

Node-graph internals (7)

AddPredefinedNodeAsync · RemovePredefinedNodeAsync · AddReverseReferencesAsync · AddRootNotifierAsync · RemoveRootNotifierAsync · OnReportEvent · OnReportEventAsync

Dependency category and testing strategy

Classifying the dependencies determines how much work re-cutting the seam actually is.

Dependency Category Consequence
Node graph (NodeState, in-memory dictionaries) In-process Deepenable directly; no port needed
Historian Already behind IHistorianProvider No new seam required
File system Already behind IFileSystemProvider No new seam required
Distributed address space Already behind ILocalAddressSpace No new seam required

Because the core is in-process and every external dependency already sits behind its own provider seam, no new port is required. The deepened module is tested directly through its narrowed interface. This is the cheap case.

Testing should replace, not layer. Tests that drive 93 virtuals by subclassing are testing past the interface and will not survive the change:

Test file Lines
NodeManagerLifecycleTests.cs 8,406
AsyncCustomNodeManagerTests.cs 5,821
MasterNodeManagerNodeManagementTests.cs 2,499
MasterNodeManagerDeterministicTests.cs 1,639
CustomNodeManagerDeterministicTests.cs 1,355

New tests should assert observable outcomes through the narrow interface so they survive internal refactors.

Proposed target shape

Nothing moves. The 8,028 lines of AsyncCustomNodeManager stay exactly where they are. Only the interface is re-cut:

  • 5 mandatory members for authoring a node manager.
  • 1 opt-in INodeManagerObserver replacing the 9 scattered notification hooks.
  • The 15 existing I*AsyncNodeManager capability interfaces remain the opt-in route for service-set behaviour, with the duplicate base-class virtuals removed.
  • 2 provider hooks converted to injection.
  • 36 members made private or sealed.

Net: 93 extension points → 5 mandatory members, with the same behaviour behind them.

The strongest argument for the shape is that this assembly already contains the answer twice. IHistorianProvider (2 members over ~5,900 lines) and IMonitoredItemManager (7 members) demonstrate the target pattern, and they sit directly alongside the 24 virtuals that duplicate them.

Constraints and risks

  • Backward compatibility. The repository requires compatibility with 1.5.378 (master378); replaced API must be marked [Obsolete] rather than removed. This makes the change additive. INodeManager.cs currently contains zero [Obsolete] markers, so the deprecation pass has not begun. The existing SyncNodeManagerAdapter and AsyncNodeManagerAdapter are the natural compatibility mechanism.
  • FluentNodeManagerBase layers rather than replaces. It derives from AsyncCustomNodeManager and therefore inherits all 93 members, adding EventSourceRegistry and SimulationRegistry on top. It would need re-basing onto the narrow seam rather than stacking on the wide one.
  • Source generators emit against this surface. The generated NodeManagerBase derives from FluentNodeManagerBase; generator templates must move in lockstep. See NodeManagers.md.
  • SampleNodeManager implements INodeManager directly and is the one external adapter for that interface. It either migrates or stays as the demonstration of the obsolete path.

Open questions

  1. Should the 5-member authoring seam be an interface (INodeSource-style, favouring composition) or remain an abstract base class (favouring the existing 33 subclasses)? This is the main design decision and is worth exploring more than once.
  2. Should GetManagerHandle's opaque object handle be replaced with a typed handle struct, and can that be done without breaking the Processed-flag multiplexing that MasterNodeManager depends on?
  3. Can the Processed cooperative-multiplexing protocol be replaced by explicit partitioning in MasterNodeManager, removing the contract from the seam entirely?
  4. Does the sync/async duality still need two adapters, or can INodeManager become a pure [Obsolete] compatibility shim over IAsyncNodeManager?

Reproducing the evidence

Run from the repository root in PowerShell.

Enumerate the virtual/abstract surface:

$lines = Get-Content src\Opc.Ua.Server\NodeManager\AsyncCustomNodeManager.cs
($lines | Select-String '^        (public|protected)(\s+internal)?\s+(virtual|abstract)\s+').Count

Count the override census by consumer category:

$files = Get-ChildItem 'src','samples','tests' -Recurse -Filter *.cs -File |
    Where-Object { $_.FullName -notmatch '\\obj\\|\\bin\\' }
$rows = @()
foreach ($f in $files) {
    $t = Get-Content -LiteralPath $f.FullName -Raw
    if ($t -notmatch ':\s*(AsyncCustomNodeManager|FluentNodeManagerBase|CoreNodeManager)\b') { continue }
    $rel = $f.FullName.Replace("$PWD\", '')
    $cat = if ($rel -like 'src\Opc.Ua.Server\*') { 'core' }
           elseif ($rel -like 'src\*') { 'sib' }
           elseif ($rel -like 'samples\*') { 'sample' }
           else { 'test' }
    foreach ($m in [regex]::Matches($t, '\boverride\s+[\w\<\>\?\[\]\.,\s]{0,80}?(\w+)\s*[\(\{]')) {
        $rows += [PSCustomObject]@{ Name = $m.Groups[1].Value; Cat = $cat }
    }
}
$rows | Group-Object Name | Sort-Object Count -Descending

Count direct INodeManager implementations outside the framework:

Get-ChildItem 'src','samples','tests' -Recurse -Filter *.cs -File |
    Where-Object { $_.FullName -notmatch '\\obj\\|\\bin\\' } |
    Select-String -Pattern '(class|record)\s+\w+\s*:[^{]*\bINodeManager\b'

See also