# Description Remove stock phrasing and correct misleading or inconsistent technical documentation. The initial two passes covered 367 Markdown files, followed by an interactive review of 169 editorial flags. The U5 interpretation originally deferred in that review has since received the upstream Mantis answer recorded by #4843; remaining server/spec follow-ups stay explicit in the CTT log. - Correct package responsibilities, migration steps, API names, ownership, protocol descriptions and links. Keep bundled migration guides synchronized. Preserve historical conclusions as historical records with separate re-check notes. - Provide at least two rendered TOC levels throughout `docs/`. Convert appropriate architecture figures to Mermaid without changing code examples, transcripts or historical quotations. - Address the October 19 feedback with concrete setup/migration instructions, corrected certificate paths and PubSub names, shorter repeated caveats, consistent XML comments, and a full review of the Concepts guide. Retain #4839's API corrections. - Integrate #4841's runtime/performance guidance and review incoming/recent prose. Preserve dated measurements rather than claiming new benchmark runs; clarify allocation ratios, startup-only processor settings, JIT warm-up and the benchmark commit's scope. Missing raw measurement artifacts and repeat ranges remain tracked by #4842. - Merge master through `e8c5d800a` in `32c6394b7`. Resolve the CTT log conflict using the updated Mantis states and OPC answers, retaining historical framing and re-checks for C28/C32/C43. - Address the latest naming/index feedback in `adb5bf693`: rename the guides to `docs/WotCon.md` and `docs/WotNodeSetConversion.md`; move the provider and DataType-declaration contracts into the conversion guide and remove `docs/WoT.md`. Rename `samples/DI` to `samples/Di` and `samples/Isa95` to `samples/ISA95`. Add content-index READMEs to Di, FluentApi, ISA95, Lds and PubSub; update solution entries, project references, Docker/CI paths, test fixture paths and documentation links with exact casing. **WoT guide scope.** This branch retains the detailed conversion guide that #4758 had replaced with an anchor stub. Conversion limits, verified document sets, preservation rules and provider ownership now live together in `WotNodeSetConversion.md`; binding/runtime guidance remains in `WotBindings.md` and `WotCon.md`. The design-rationale/test-ledger prose was shortened, the oversized argument-table cell moved into its section, and existing anchors preserved. **SDK policy is a separate, explicit user-directed change.** `global.json` sets a stable minimum of `10.0.400`, `rollForward: latestMajor`, and `allowPrerelease: false`. Newer major SDKs are permitted; CI's explicit SDK pins are unchanged. README, GettingStarted, DeveloperGuide and Recycling instructions reflect the possible analyzer differences. The image-contract regression fix accepts compatible stable SDK images instead of requiring exact equality with the floor. The latest four rename/index threads are resolved after publication. Three older threads remain open: universal obsolete-removal timing, unconditional nonce-strength wording, and a blanket Vision NativeAOT claim. Existing implementation/evidence constraints are explained in their inline replies. The two-level TOCs remain as requested. ## Validation - `git diff --check` and staged diff checks: passed. The untouched upstream AutoID NodeSet retains its provenance bytes, including two existing license-header trailing spaces. - `.\.azurepipelines\validate-migration-plugin.ps1`: passed; 15 bundled migration documents and manifests synchronized. - Documentation/path validation: **101** documents retain two rendered TOC levels (**2,228** entries), with idempotent formatting. **52** new relative links and their anchors passed exact-case checks. No tracked references to the old guide/sample paths remain. Both relocated provider-contract sections are preserved in the destination. - `dotnet sln UA.slnx list`: passed; shows the renamed Di/ISA95 projects and the existing FluentApi/Lds/PubSub projects. Updated solution and pump ProjectReference paths also passed exact-case existence checks. - Latest focused commands, run from the repository root: ```powershell # Passed: 61 channel send-order and reconnect tests. dotnet test tests\Opc.Ua.Core.Tests\Opc.Ua.Core.Tests.csproj -p:CustomTestTarget=net10.0 --filter 'FullyQualifiedName~UaSCBinaryChannelSendOrderTests|FullyQualifiedName~TcpReconnectKeyAgreementTests' --verbosity minimal # Passed: 5 pump datasheet tests; builds the renamed pump sample. dotnet test tests\Opc.Ua.Di.Tests\Opc.Ua.Di.Tests.csproj -p:CustomTestTarget=net10.0 --filter 'FullyQualifiedName~PumpDatasheetConformanceTests' --verbosity minimal # Passed: 4 repository NodeSet path/round-trip tests, including the moved pump models. dotnet test tests\Opc.Ua.Types.Tests\Opc.Ua.Types.Tests.csproj -p:CustomTestTarget=net10.0 --filter 'FullyQualifiedName~RepositorySourceSurvivesSerializedLinkedPartitionsAsync' --verbosity minimal # Passed: build, zero warnings/errors. dotnet build samples\ISA95\MinimalIsa95Server\MinimalIsa95Server.csproj -p:CustomTestTarget=net10.0 -f net10.0 --verbosity minimal # Passed: dependent sample-test project build, zero warnings/errors. dotnet build tests\Opc.Ua.WotCon.Samples.Tests\Opc.Ua.WotCon.Samples.Tests.csproj -p:CustomTestTarget=net10.0 -f net10.0 --verbosity minimal ``` Earlier focused integration checks passed 2,620 tests across AutoID, event queues/scheduling, source generation, MCP/package profiles and WoT. Both reviewed Mermaid diagrams rendered locally without network access. These checks are not a full solution run. The full solution has **not** been rerun for this review. Earlier `dotnet test UA.slnx -p:CustomTestTarget=net10.0 --verbosity quiet` did not pass: three AOT projects rejected VSTest and 20 Vision tests failed during graphics initialization with `OutOfMemoryException`. Remaining long-running tests were stopped at the user's request. A focused Vision rerun passed 558 tests and reproduced those 20 failures. The earlier `dotnet test UA.slnx -p:CustomTestTarget=net48 --verbosity quiet` was blocked by the former SDK pin and was not retried after the policy change. The user approved proceeding with those limitations documented; current remote CI is not claimed green. ## Related Issues - Follow-up #4842 tracks the requested 2.0.2+ benchmark/tuning and diagnostic-tool documentation refresh. #4841 supplies documentation and earlier results, not all newly requested measurements. - #4758 explains the conversion guide's earlier consolidation; the current provider contracts are now consolidated into `WotNodeSetConversion.md`. - #4839 supplies the reviewed API corrections; #4843 supplies the updated CTT issue record. - The original prose cleanup and SDK policy were user-requested without a source issue. No new runtime architecture is introduced by these documentation edits. ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [ ] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Roman Ettlinger <romanett98@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
34 KiB
Server Runtime Seam Analysis
Status: historical design analysis with follow-up decisions. This document records an evidence-based assessment of the server runtime cluster in
Opc.Ua.Server:StandardServer,ServerInternalData,Session,SessionManager,Subscription,SubscriptionManager,ApplicationInstance, and related machinery. The findings describe commite73e71184; subsequent implementation and rejected recommendations are recorded under Open questions. This document is not a current API inventory. For shipped documentation see Sessions.md and Subscriptions.md.
Analysis performed against commit e73e71184 on master.
Explicitly out of scope: node managers (INodeManager, AsyncCustomNodeManager,
CustomNodeManager2, MasterNodeManager) and node states. These are plugin API and
breaking them is not in play. They are assessed separately in
NodeManagerAnalysis.md.
Table of contents
- Purpose
- Vocabulary
- Method
- Surface inventory
- Finding 1: the diagnostics lock leak
- Finding 2: IServerInternal is a service locator
- Finding 3: ISubscription publishes its own state machine
- Finding 4: ISubscriptionManager re-declares ISubscription
- Finding 5: ISession mixes five concerns
- Finding 6: ISessionManager extends through events
- Finding 7: IApplicationInstance
- What is healthy — do not touch
- Deletion tests
- Dependency categories and testing strategy
- Ranked recommendations
- Constraints and risks
- Open questions
- Reproducing the evidence
- See also
Purpose
The node-manager surface is frozen as plugin API. That leaves the server runtime — the machinery a plugin author never implements but every request flows through — as the remaining place where the seams can still be re-cut without breaking consumers.
This document quantifies where that cluster's interfaces are mis-shaped, and separates the parts that are well-designed from the parts that are not. Two of the seven findings recommend no change; that is deliberate.
Vocabulary
The analysis uses deep-module design terms consistently:
| Term | Meaning here |
|---|---|
| Module | Anything with an interface and an implementation. |
| Interface | Everything a caller must know to use the module correctly: the type signature plus invariants, ordering constraints, error modes, and side effects. |
| Depth | Leverage at the interface — behaviour obtained per unit of interface learned. |
| Seam | The place where a module's interface lives. |
| Adapter | A concrete thing satisfying an interface at a seam. |
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 gathered mechanically over src, samples, and tests, excluding obj and
bin. Interface member counts come from language-server document symbols rather than
regular expressions, so interface members without access modifiers are counted correctly.
Lock-statement counts come from a targeted pattern match per file. Commands are listed
under Reproducing the evidence.
Surface inventory
| Module | Interface members | Implementation | Assessment |
|---|---|---|---|
IServerInternal |
57 (34 properties, 23 methods) | ServerInternalData — 1,355 lines, 70 public |
Service locator |
ISubscription |
42 | Subscription — 3,471 lines, 51 public |
Publishes its own state machine |
ISession |
37 | Session — 1,526 lines, 41 public |
Five concerns in one interface |
ISubscriptionManager |
26 (2 events, 24 methods) | SubscriptionManager — 2,858 lines, 60 public |
Re-declares ISubscription |
ISessionManager |
18 (7 events, 11 methods) | SessionManager — 1,940 lines, 29 public |
Extends through events |
IApplicationInstance |
17 | ApplicationInstance — 1,387 lines, 36 public |
Three concerns, narrower than its class |
IStandardServer |
9 | StandardServer — 5,575 lines, 96 public / 53 protected / 37 virtual |
Healthy |
Finding 1: the diagnostics lock leak
An object-typed lock is published on three public interfaces —
IServerInternal.DiagnosticsLock, IServerInternal.DiagnosticsWriteLock,
ISession.DiagnosticsLock, ISubscription.DiagnosticsLock,
ISubscription.DiagnosticsWriteLock — and taken in 88 lock statements across
7 files:
| File | lock statements |
|---|---|
Server/StandardServer.cs |
31 |
Subscription/Subscription.cs |
28 |
Subscription/SubscriptionManager.cs |
15 |
Session/Session.cs |
9 |
Session/SessionManager.cs |
2 |
NodeManager/CoreNodeManager.cs |
1 |
samples/Reference/ConsoleReferenceServer/UAServer.cs |
2 |
| Total | 88 |
The sample-code entry matters: this is not an internal detail, it is a lock that external consumers have been shown how to acquire.
The lock is the guarded public data object
// Subscription.cs:415
public object DiagnosticsLock => Diagnostics;
// Session.cs:332
public object DiagnosticsLock => SessionDiagnostics;
ISubscription.Diagnostics (a SubscriptionDiagnosticsDataType) and
ISession.SessionDiagnostics (a SessionDiagnosticsDataType) are themselves public
interface members. The lock object and the data it guards are the same reference, and both
are publicly reachable. Any caller can write lock (subscription.Diagnostics) — never
touching DiagnosticsLock at all — and contend with the server's internal critical
sections.
The lock getter has a side effect that runs outside the lock
// ServerInternalData.cs:656 — and structurally identical in Subscription.cs:420
public object DiagnosticsWriteLock
{
get
{
// implicitly force diagnostics update
DiagnosticsNodeManager?.ForceDiagnosticsScan();
return DiagnosticsLock;
}
}
Therefore every one of the 31 lock (ServerInternal.DiagnosticsWriteLock) sites in
StandardServer performs an unsynchronised write to diagnostics-scan state before the
lock is acquired — unsynchronised with respect to the very lock it is about to hand out.
ForceDiagnosticsScan() is a single flag assignment
(DiagnosticsNodeManager.cs:700-703, m_forceDiagnosticsScan = true); the flag is consumed
by the attribute read paths and the periodic scan timer (:1772, :1798, :1895-1898),
which run one fresh scan on demand. The comments at :1008-1011 and :1145-1148 show the
flag exists precisely to avoid a synchronous O(N²) scan. So the cost on the lock path is a
bool write, not a rescan — but it is still a side effect on a property getter, and still a
racy write on a path whose entire purpose is synchronisation.
Nothing in the type signature object DiagnosticsWriteLock { get; } reveals this. Under
the definition of interface used here, the real interface of this member is
"reading this property marks the diagnostics arrays for a forced rescan, then returns a lock
aliased to the diagnostics data object" — none of which is expressible in, or visible
from, the declaration.
Cross-object lock nesting with no documented order
SubscriptionManager acquires four distinct lock objects, nested, with no stated
hierarchy:
lock (m_server.DiagnosticsWriteLock) // SubscriptionManager.cs:476, 563, 747, 925, 1331
lock (context.Session.DiagnosticsLock) // 756, 935, 1158, 1261, 1828, 2036, 2101
lock (ownerSession.DiagnosticsLock) // 1843
lock (subscription.DiagnosticsLock) // 1498, 1905
Several of these are nested inside one another (for example m_server.DiagnosticsWriteLock
at 747 enclosing context.Session.DiagnosticsLock at 756). No lock-ordering contract is
documented anywhere, which is precisely the situation the repository rule against exposing
locks exists to prevent.
Tests cannot catch any of this
// tests/Opc.Ua.Server.Tests/SubscriptionTests.cs:70, 86
m_serverMock.Setup(s => s.DiagnosticsWriteLock).Returns(new object());
m_sessionMock.Setup(s => s.DiagnosticsLock).Returns(new object());
A mocked lock returns an object that nothing else in the system contends on. The tests
therefore exercise zero mutual exclusion — and, because the mock replaces the property,
they also never execute the ForceDiagnosticsScan() flag write. A diagnostics race or a
lock-ordering deadlock cannot be reproduced by this suite.
Rules violated
This one design decision breaches three explicit repository rules:
- "NEVER expose 'locks' or locking in any internal or public API surface because nobody can effectively reason over the lock behavior."
- "NEVER use
private readonly object m_lock = new()or any otherobject-typed sync root. All synchronous locks MUST useSystem.Threading.Lock." - "NEVER use
objectorobject?in public API unless overridingEquals."
The System.Threading.Lock polyfill already exists at
src/Opc.Ua.Types/Polyfills/System.Threading.cs:85 and is available on every supported
TFM, so the replacement type is in place today.
The fix pattern already exists on the same interface.
IServerInternal.UpdateServerStatus(Action<ServerStatusValue>) demonstrates exactly the
right shape: the module owns its lock, the caller passes a delegate, and no sync root
crosses the seam. Applying that shape to diagnostics removes all 88 lock statements, both
side-effecting getters, and all five leaked members.
The wider inventory — this leak is repo-wide
The diagnostics locks are the largest instance, not the only one. A sweep for object-typed
lock members exposed as public or protected properties finds ten across three assemblies:
| Assembly | Member | Kind |
|---|---|---|
Opc.Ua.Core |
AsyncResultBase.Lock (:144) |
public object |
Opc.Ua.Core |
UaSCBinaryChannel.DataLock (:1045) |
protected object |
Opc.Ua.Core |
ApplicationConfiguration.PropertiesLock => m_properties (:104) |
lock is the guarded data |
Opc.Ua.Types |
BaseVariableState.Lock (:2122) |
public object |
Opc.Ua.Types |
NodeBrowser.DataLock (:242) |
protected object |
Opc.Ua.Types |
Node.DataLock => this (:476) |
lock on this |
Opc.Ua.Server |
ServerInternalData.DiagnosticsLock (:649) |
public object |
Opc.Ua.Server |
Subscription.DiagnosticsLock => Diagnostics (:415) |
lock is the guarded data |
Opc.Ua.Server |
Session.DiagnosticsLock => SessionDiagnostics (:332) |
lock is the guarded data |
Opc.Ua.Server |
CustomNodeManager.Lock (:237) |
public object |
Node.DataLock => this and ApplicationConfiguration.PropertiesLock => m_properties are the
same defect class as Subscription.DiagnosticsLock => Diagnostics: the published sync root
is the guarded object, so a caller can contend without ever naming the lock member.
The historical example below uses the required System.Threading.Lock type,
but still exposes its synchronization object. It satisfies the lock-type rule,
not the rule against exposing locks:
Opc.Ua.Redundancy.Server/Subscriptions/SharedKeyValueSubscriptionStore.cs:1467:
public Lock Lock { get; } = new();
The owner-side operation pattern described above avoids both violations.
There is a third lock leak beyond these ten, documented separately because it is a
published contract rather than a member: NodeState.ReadAttributeAsync and
WriteAttributeAsync take lock (this) under a CA2002/RCS1059 suppression whose comment
states "external callers synchronise via lock(source)", and 15 call sites honour it —
6 lock(this) inside the node types (BaseVariableState 4, NodeState 2) and 9 external
lock(source)/lock(node) statements (AsyncCustomNodeManager 8, CustomNodeManager 1).
A naive text search reports 19; ten of those matches are the TODO comments and #pragma
lines that mention lock(source) rather than take it. The command below excludes them.
That contract cannot cross a replica and must be removed before any new node representation
exists.
Reproduce the sweep:
# the ten object-typed lock members
Get-ChildItem src -Recurse -Filter *.cs -File |
Where-Object { $_.FullName -notmatch '\\obj\\|\\bin\\' } |
Select-String -Pattern '^\s*(public|protected|internal)\s+object\s+\w*[Ll]ock\w*\s*(\{|=>)'
# the 15 lock-contract sites, excluding comment and pragma mentions
$files = Get-ChildItem src -Recurse -Filter *.cs -File |
Where-Object { $_.FullName -notmatch '\\obj\\|\\bin\\' }
$sites = @()
foreach ($f in $files) {
$n = 0
foreach ($line in [IO.File]::ReadAllLines($f.FullName)) {
$n++
if ($line -match 'lock\s*\(\s*(this|source|node|m_node)\s*\)') {
$trim = $line.TrimStart()
if (-not ($trim.StartsWith('//') -or $trim.StartsWith('*') -or
$trim.StartsWith('#pragma'))) {
$sites += "$($f.Name):$n $($line.Trim())"
}
}
}
}
$sites.Count # 15
$sites
Finding 2: IServerInternal is a service locator
IServerInternal has 57 members: 34 properties and 23 methods.
The 34 properties hand out more than 20 subsystems — NodeManager, CoreNodeManager,
DiagnosticsNodeManager, ConfigurationNodeManager, MainNodeManagerFactory,
EventManager, ResourceManager, RequestManager, AggregateManager,
ModellingRulesManager, ConformanceUnitsManager, SessionManager,
SubscriptionManager, RoleManager, IdentityRegistry, UserManagement,
MonitoredItemQueueFactory, SubscriptionStore, Telemetry, ServerObject, and more.
Twelve of the 23 methods are Set* mutators: SetNodeManager,
SetMainNodeManagerFactory, SetSessionManager, SetMonitoredItemQueueFactory,
SetSubscriptionStore, SetRoleManager, SetIdentityRegistry, SetUserManagement,
SetAggregateManager, SetModellingRulesManager, SetConformanceUnitsManager, and
UpdateServerStatus.
Three consequences:
-
Depth is zero. The interface is the implementation surface.
ServerInternalDatais 1,355 lines of largely property storage behind a 57-member interface. -
It is the universal ambient handle.
IServerInternalis referenced across more than 200 files insrc,samples, andtests;NodeManagerLifecycle.csalone references it 55 times. Anything holding it can reach anything else in the server. -
Two-phase construction with implicit ordering.
SetNodeManagersilently reaches into its argument to populate three further properties:// ServerInternalData.cs:338 public void SetNodeManager(IMasterNodeManager nodeManager) { NodeManager = nodeManager; DiagnosticsNodeManager = nodeManager.DiagnosticsNodeManager!; ConfigurationNodeManager = nodeManager.ConfigurationNodeManager!; CoreNodeManager = nodeManager.CoreNodeManager!; }and
SetSessionManagerunhooks previously registered event handlers, so it is designed to be called more than once. The required call order is interface complexity documented nowhere in the types. Mutable-after-construction global state is also directly hostile to the stated high-availability and distributed-server goals (see HighAvailability.md).
However: IServerInternal is what every node manager receives. Narrowing it is a
plugin-API break and therefore out of scope. The recommendation is containment, not
re-cutting — see Ranked recommendations.
Finding 3: ISubscription publishes its own state machine
Of ISubscription's 42 members, eleven are collaboration protocol with
SubscriptionManager and SessionPublishQueue rather than caller-facing API:
ItemReadyToPublish · ItemNotificationsAvailable · QueueOverflowHandler ·
PublishTimerExpired · PublishTimeout · SubscriptionTransferred ·
AvailableSequenceNumbersForRetransmission · Acknowledge · ResendData ·
SessionClosed() · SessionClosed(ISession)
These sit on the public interface, where any holder of an ISubscription — including a
node manager reaching through IServerInternal.SubscriptionManager.GetSubscriptions() —
can call PublishTimerExpired() or QueueOverflowHandler() directly and corrupt the
publishing state machine.
The behaviour behind them is real and substantial: Subscription (3,471 lines),
SessionPublishQueue (1,071), and SentMessageQueue (449) are roughly 5,000 lines of
genuine publishing machinery. This is the strongest deepenable candidate in scope —
moving the publish-pipeline protocol to an internal seam takes ISubscription from 42 to
roughly 25 members with no behaviour change and no plugin-API impact.
Finding 4: ISubscriptionManager re-declares ISubscription
Ten of ISubscriptionManager's 26 members restate an ISubscription operation with an
extra uint subscriptionId parameter: CreateMonitoredItemsAsync,
ModifyMonitoredItemsAsync, DeleteMonitoredItemsAsync, SetMonitoringModeAsync,
SetTriggering, SetPublishingMode, Republish, ConditionRefresh,
ConditionRefresh2, DeleteSubscriptionAsync.
Each implementation is a dictionary lookup followed by delegation:
// SubscriptionManager.cs:2050
public ValueTask<ModifyMonitoredItemsResponse> ModifyMonitoredItemsAsync(
OperationContext context,
uint subscriptionId,
TimestampsToReturn timestampsToReturn,
ArrayOf<MonitoredItemModifyRequest> itemsToModify,
CancellationToken cancellationToken = default)
{
// find subscription.
if (!m_subscriptions.TryGetValue(subscriptionId, out ISubscription? subscription))
{
throw new ServiceResultException(StatusCodes.BadSubscriptionIdInvalid);
}
// modify the items.
return subscription.ModifyMonitoredItemsAsync(
context, timestampsToReturn, itemsToModify, cancellationToken);
}
Applying the deletion test to these ten members: deleting them moves an
id-to-subscription resolution into the caller. That resolution is needed —
it is where BadSubscriptionIdInvalid is produced — so the members are not pure waste, but
they are a thin routing layer that doubles the surface a reader must learn. A single
TryGetSubscription-plus-operate shape (which ISubscriptionManager.TryGetSubscription
already provides) would collapse ten members into one.
Finding 5: ISession mixes five concerns
ISession's 37 members span identity and authentication (8), continuation points (4),
diagnostics (2), lifecycle (5), request validation (3), plus certificates, nonces, locales,
and endpoint description.
Specific problems:
- Two more
objectleaks.RestoreHistoryContinuationPoint(ByteString) : object?andSaveHistoryContinuationPoint(Guid, object)put untyped state in a public interface, against the repository rule. - Continuation points already have a seam.
SessionContinuationPoints.cs(472 lines) andSession/Persistence/IContinuationPointStore.csexist. The four continuation-point members onISessionare a pass-through to a seam that is already in place, so they can be routed rather than reimplemented. - Paired sync and async variants with divergent signatures.
ValidateBeforeActivateAsyncreturnsValueTask<(IUserIdentityTokenHandler, UserTokenPolicy?)>while the synchronousValidateBeforeActivatetakes those same two values as trailing parameters. A caller must learn both shapes.
Finding 6: ISessionManager extends through events
Seven of ISessionManager's 18 members are events: SessionCreated, SessionActivated,
SessionClosing, SessionDiagnosticsChanged, SessionChannelKeepAlive,
ImpersonateUser, ValidateSessionLessRequest.
The last two are not notifications — they are behavioural hooks. ImpersonateUser carries
ImpersonateEventArgs with settable Identity, EffectiveIdentity, and
IdentityValidationError, so a handler decides the authentication outcome by mutating
event args. ValidateSessionLessRequest works the same way through its Error property.
Extension through mutable event args is invisible to the type system, unordered when
multiple handlers subscribe, and cannot express failure other than by side effect. The
repository already has the right pattern for this: pluggable identity providers and
IUserTokenAuthenticator (see IdentityProviders.md).
These two events predate that model and should route to it.
Finding 7: IApplicationInstance
IApplicationInstance has 17 members covering three concerns: configuration loading
(LoadApplicationConfigurationAsync ×3, Build), certificate lifecycle
(CheckApplicationInstanceCertificatesAsync, DeleteApplicationInstanceCertificateAsync,
AddOwnCertificateToTrustedStoreAsync, CertificatePasswordProvider,
DisableCertificateAutoCreation), and server lifecycle (StartAsync, StopAsync,
Server).
Two observations, both minor:
- The interface is narrower than the class (17 versus 36 public members) — the
certificate-prompt surface (
MessageDlg,ApproveMessageAsync) andCertificateManagerare onApplicationInstancebut not onIApplicationInstance. That is the right direction and needs no change. - Return types are inconsistent across overloads of the same method.
LoadApplicationConfigurationAsync(Stream, bool, CancellationToken)returnsTask<T>while the(bool, …)and(string, bool, …)overloads returnValueTask<T>. LikewiseStartAsyncreturnsTaskandStopAsyncreturnsValueTask. Harmless at runtime, but it is interface complexity a caller must memorise.
The certificate half of this class overlaps CertificateManager (see
CertificateManager.md), which is the deeper seam, but
IApplicationInstance is a widely used entry point and the overlap is not currently
causing duplication of logic. Third tier; no action recommended.
For completeness: the synchronous Stop() is sync-over-async
(Server?.StopAsync().AsTask().GetAwaiter().GetResult()), but it is already marked
[Obsolete("Use StopAsync")], which is the correct handling under the compatibility policy.
No action needed.
What is healthy — do not touch
IStandardServer is 9 members. StandardServer declares 37 virtual/abstract
members, and its three real (non-test) subclasses — GlobalDiscoverySampleServer,
DependencyInjectionStandardServer, and ReverseConnectServer — override 16 of them:
CreateMainNodeManagerFactory · CreateMasterNodeManagerAsync ·
CreateMonitoredItemQueueFactory · CreateRoleManager · CreateSessionManager ·
CreateSubscriptionManager · CreateSubscriptionStore · Dispose ·
LoadServerProperties · OnConnectionStatusChanged · OnNodeManagerStarted ·
OnRequestValidatedAsync · OnServerStarted · OnServerStarting ·
OnServerStoppingAsync · OnUpdateConfigurationAsync
That is a 43% utilisation rate on a clean factory-method-plus-lifecycle-hook shape, with a 9-member interface in front of 5,575 lines of implementation. It is the best-shaped seam in the cluster and should be left alone. (For contrast, the node-manager base class runs at 25 of 80, with only about a dozen caller-facing — see NodeManagerAnalysis.md.)
Deletion tests
| Module | Delete it | Verdict |
|---|---|---|
| The five diagnostics lock members | Nothing reappears — the guarded state is owned by Subscription / Session / ServerInternalData themselves, and UpdateServerStatus(Action<T>) already shows the replacement shape |
Pure leak. Remove. |
ISubscription publish-protocol members (11) |
Complexity reappears — but only inside SubscriptionManager and SessionPublishQueue, which are the only legitimate callers |
Move to an internal seam. |
ISubscriptionManager routing members (10) |
Id-to-subscription resolution moves to callers; BadSubscriptionIdInvalid handling would be duplicated |
Thin, but not free. Collapse rather than delete. |
ISession continuation-point members (4) |
Nothing reappears — SessionContinuationPoints and IContinuationPointStore already hold the behaviour |
Route to the existing seam. |
IServerInternal |
Complexity reappears everywhere; 200+ files depend on it | Load-bearing, but for the wrong reason. Contain, do not re-cut. |
IStandardServer / StandardServer virtuals |
Complexity reappears in all three subclasses | Earns its keep. |
Dependency categories and testing strategy
| Dependency | Category | Consequence |
|---|---|---|
| Diagnostics data structures | In-process | Deepenable directly; no port needed |
| Publish pipeline, sent-message queue | In-process | Internal seam only |
| Continuation points | Already behind IContinuationPointStore |
Seam exists |
| Subscription persistence | Already behind ISubscriptionStore |
Seam exists |
| Monitored item queues | Already behind IMonitoredItemQueueFactory |
Seam exists |
Every dependency in this cluster is either in-process or already behind a provider seam, so no new port is required for any recommendation below.
Testing should replace, not layer. The current suite mocks the diagnostics locks, which
means it verifies neither mutual exclusion nor the ForceDiagnosticsScan flag write. Tests
for the replacement should assert observable diagnostics outcomes through the owning
module's update method, so they survive the internal change.
Ranked recommendations
| # | Work | Plugin-API break? | Payoff |
|---|---|---|---|
| 1 | Replace the five diagnostics lock members with owner-side update methods backed by System.Threading.Lock |
No — these members are server-internal, not node-manager-facing | Removes 88 lock statements, a real deadlock surface, two side-effecting getters, and three rule violations |
| 2 | Move the 11 publish-pipeline members off ISubscription to an internal seam |
No | 42 → ~25 members over ~5,000 lines of real behaviour |
| 3 | Route the 4 ISession continuation-point members through the existing IContinuationPointStore; remove the two object leaks |
No | 37 → ~33, kills two object leaks |
| 4 | Collapse the 10 routing members on ISubscriptionManager behind TryGetSubscription |
No | 26 → ~17 |
| 5 | Route ImpersonateUser / ValidateSessionLessRequest to IUserTokenAuthenticator |
Yes, for those two events | Removes mutable-event-args extension |
| 6 | Freeze IServerInternal; new subsystems get injected interfaces rather than another property |
Deferred | Stops the growth without a break |
Item 1 is the recommended starting point: highest severity, strongest evidence, no plugin impact, and the replacement pattern already exists on the same interface.
Constraints and risks
- Backward compatibility. The repository requires compatibility with 1.5.378
(
master378); replaced API must be marked[Obsolete]rather than removed. All recommendations are therefore additive in the first pass. ConsoleReferenceServertakes the lock.samples/Reference/ConsoleReferenceServer/UAServer.cshas twolock (session.DiagnosticsLock)sites. The sample must migrate to the replacement method in the same change, since it is the worked example consumers copy.- Tests mock the locks.
SubscriptionTests.csandSessionSecurityTests.csset upDiagnosticsLock/DiagnosticsWriteLockon mocks. Those setups must be removed rather than adapted, or they will silently keep passing against a removed concept. Subscription.DiagnosticsLock => Diagnosticsmeans the lock and data are one object. Any migration must confirm no external code relies on lockingDiagnosticsdirectly.IServerInternalis reachable from node managers, so anything reached through it is indirectly plugin-visible even when the member itself is not on a node-manager interface. Each removal in item 1 needs that check.
Open questions
- Should diagnostics updates be exposed as
UpdateDiagnostics(Action<T>)(matchingUpdateServerStatus) or as intention-revealing named methods (RecordRequestCompleted,RecordSubscriptionTransferred, …)? The latter is deeper but is a larger change. - Does the
ForceDiagnosticsScan()flag write need to happen at all 88 sites, or is it a blanket safety net that a narrower set of explicit invalidation points would replace? (Note it is aboolassignment, not a scan — see the side-effect section.) Can the publish-pipeline protocol become an internal interface consumed only byAnswered (issue #4185): yes, and restore needs no external access. The persistence seam (SubscriptionManagerandSessionPublishQueue, or does durable-subscription restore (see DurableSubscription.md) require external access?Subscription/Persistence/) never referencesISubscription— the store deals inIStoredSubscriptionDTOs and restore constructs the concrete type viaSubscription.RestoreAsyncinside the manager. The eight live pipeline members moved to the internalISubscriptionPublishPipeline(implemented explicitly bySubscription),ItemReadyToPublish/ItemNotificationsAvailablewere deleted as dead (bodies commented out since 1.5.x), andSessionPublishQueuebecame internal.ResendDatastayed public: its caller is theServer_ResendDatamethod handler, not the pipeline.- Is there an existing documented lock hierarchy for the four nested diagnostics locks, or has ordering been maintained by convention only?
ShouldAnswered (issue #4186): keep the flat surface. Measurement showed onlyISubscriptionManager's routing members be collapsed, or is the flat id-addressed surface load-bearing for the service-set dispatch inStandardServer?Republish,SetTriggering,ModifyMonitoredItemsAsyncandSetMonitoringModeAsyncare pure lookup-and-delegate (one caller each, inStandardServer); the other six carry logic with noISubscriptionsubstitute (session monitored-item-count accounting, theSetPublishingModebatch loop, the manager-private condition-refresh queue, theDeleteSubscriptionAsyncorchestrator). A collapse of the four was prototyped and rejected: it traded ten uniform id-addressed members for a mixed dispatch shape inStandardServerwithout reducing what a reader must understand. Every service keeps going throughISubscriptionManager.
Reproducing the evidence
Run from the repository root in PowerShell.
Count the diagnostics lock statements per file:
$total = 0
$files = @(
'src\Opc.Ua.Server\Server\StandardServer.cs',
'src\Opc.Ua.Server\Subscription\Subscription.cs',
'src\Opc.Ua.Server\Subscription\SubscriptionManager.cs',
'src\Opc.Ua.Server\Session\Session.cs',
'src\Opc.Ua.Server\Session\SessionManager.cs',
'src\Opc.Ua.Server\NodeManager\CoreNodeManager.cs',
'samples\Reference\ConsoleReferenceServer\UAServer.cs')
foreach ($f in $files) {
$c = [IO.File]::ReadAllText($f)
$n = ([regex]::Matches($c, 'lock\s*\([^)]*Diagnostics(Write)?Lock\s*\)')).Count
$total += $n
'{0,-40} {1,3}' -f (Split-Path $f -Leaf), $n
}
"TOTAL: $total"
Find every declaration and use of the leaked locks:
Get-ChildItem 'src','samples','tests' -Recurse -Filter *.cs -File |
Where-Object { $_.FullName -notmatch '\\obj\\|\\bin\\' } |
Select-String -Pattern 'DiagnosticsLock|DiagnosticsWriteLock'
Measure the StandardServer virtual surface against its three real subclasses:
$t = [IO.File]::ReadAllText('src\Opc.Ua.Server\Server\StandardServer.cs')
'virtual/abstract declared: ' +
([regex]::Matches($t, '(?m)^ (?:public|protected)(?: internal)? (?:virtual|abstract)(?: async)? ')).Count
$names = @()
foreach ($f in 'src\Opc.Ua.Gds.Server\GlobalDiscoverySampleServer.cs',
'src\Opc.Ua.Server\Hosting\DependencyInjectionStandardServer.cs',
'src\Opc.Ua.Server\Server\ReverseConnectServer.cs') {
$c = [IO.File]::ReadAllText($f)
foreach ($m in [regex]::Matches($c, '\boverride\s+[\w<>\?\[\]\.,\s]{0,80}?(\w+)\s*[\(\{]')) {
$names += $m.Groups[1].Value
}
}
'distinct overrides: ' + ($names | Sort-Object -Unique).Count
Interface member counts were taken from language-server document symbols on
IServerInternal.cs, ISession.cs, ISubscription.cs, ISessionManager.cs,
ISubscriptionManager.cs, IStandardServer.cs, and IApplicationInstance.cs rather than
by pattern matching, because interface members carry no access modifier.
See also
- Node Manager Seam Analysis — the sibling analysis for the plugin-API surface deliberately excluded here.
- Sessions, Reconnection, and Subscription Engines — shipped architectural overview.
- Subscriptions and Monitored Items Service Set — the V2 subscription engine.
- Diagnostics — server diagnostics nodes and audit events, the data guarded by the locks in Finding 1.
- Identity Providers — the pluggable authentication model that Finding 6 recommends routing to.
- High Availability and OPC UA Redundancy — the distributed goals that Finding 2 works against.
- Dependency Injection — the injection surface referenced by recommendation 6.
- Developer Guide — coding standards cited in Finding 1.