Files
UA-.NETStandard/samples/Quickstarts.Servers/MemoryBuffer/MemoryBufferBrowser.cs
Marc Schier 1cca744b5c Retire the last exposed locks: node, browser, server diagnostics and application properties (#4222)
Closes #4201. Closes #4202. Closes #4203. Closes #4215. Closes #4216.

(#4215 and #4216 are @marcschier's originals from the #4188 review;
#4202 and #4201 are my later duplicates of the same two findings. All
four describe the two changes in section 1 and are closed together.)

**This PR now carries what was #4221 as well.** The two were a stack;
they have been unstacked and combined here, and #4221 is closed.
Everything below the "Read the server diagnostics" section is the work
that was previously reviewed as #4221.

---

## 1. The browse path guards itself, and the last node locks are gone

`AsyncCustomNodeManager` kept two `lock (source)` sites around
`CreateBrowser`, carrying a NOTE that browser creation was not
internally synchronized. `NodeBrowser` still handed its lock to derived
types through `protected Lock DataLock`.

`NodeState.CreateBrowser` now holds a browse lock across
`PopulateBrowser` and `OnPopulateBrowser`, so browser construction on a
node is serialized — which is what the external `lock (source)` was
actually providing, and what a handler that mutates the node during
population (the historian's lazy config installer) depends on. Both
`lock (source)` statements and the NOTE are gone.

**What that lock does not do**, stated plainly in the XML docs and in
`docs/NodeManagers.md` rather than over-promised: it is not an atomic
snapshot across the node's children, notifiers and references. Writers
take those collections' own locks, not the browse lock. The browser is a
point-in-time copy and each collection is read consistently, but the
combination is not a transaction.

No `lock` takes a `NodeState` as its sync root any more. That was the
last thing keeping `NodeState` from being documentably self-guarding, so
the other eleven went with it: `lock (activeNode)`, `lock (target)`,
`lock (source.Node)`, `lock (target.Node)`, `lock (notifier)`, `lock
(handle.Node)` in `AsyncCustomNodeManager`, and `lock (node)` in
`CustomNodeManager2`. Most were a check-then-act pair now served by
`NodeState.AddReferenceIfMissing`, which does the check and the insert
under the node's own lock.

`NodeBrowser.DataLock` is removed. A browser is **single-consumer**: it
belongs to whoever created it, and both server browse paths already
serialize on the owning side. The exposed lock only added an
inheritance-level locking contract that a derived browser could not
reason about.

## 2. Read the server diagnostics under the lock every writer takes

`OnUpdateDiagnostics` locked the diagnostics **payload** rather than the
lock every writer takes through `UpdateServerDiagnostics`, so the
snapshot a client reads from the `ServerDiagnostics` node was never
excluded from a concurrent writer.

Both halves matter, and the second is the one that is easy to miss:

```csharp
lock (m_diagnosticsLock)                                        // was: lock (ServerDiagnostics)
{
    value = Variant.FromStructure(ServerDiagnostics, copy: true);  // was: copy defaulted to false
}
```

`copy: true` is what makes it a snapshot. `FromStructure(value, copy:
false)` does `new ExtensionObject(value, copy)` — it wraps the **live**
structure without copying it, so the caller would read the fields long
after the lock was released and see them change underneath. Swapping the
lock alone would have looked like a fix while still excluding nothing.

## 3. Analyzer numbering

The browser rule is **UA0027**, not UA0025. This branch had
independently taken UA0025 for `NodeBrowser.DataLock` while #4188
published UA0025 for `ILocalNode.DataLock` and UA0026 for
`BaseVariableValue.Lock`. Master's are shipped, so they keep the
numbers:

| | UA0025 | UA0026 | UA0027 |
|---|---|---|---|
| master (#4188) | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | —
|
| **this PR** | `ILocalNode.DataLock` | `BaseVariableValue.Lock` |
**`NodeBrowser.DataLock`** |

Both sides had written a different `UA0025Tests.cs`, which arrived as an
add/add conflict. They are now split: `UA0025Tests.cs` holds master's
`ILocalNode` tests, `UA0027Tests.cs` holds the `NodeBrowser` ones.

UA0024's help text also drops its reference to `ReadServerDiagnostics`,
which #4193 removed as unused, and names `ISession.ReadDiagnostics` /
`ISubscription.ReadDiagnostics` instead — those do still exist.


## 4. Two more locks reachable from outside their owner

**`ApplicationConfiguration.PropertiesLock` is removed.** It returned
the properties dictionary itself, so `lock
(configuration.PropertiesLock)` was `lock (configuration.Properties)` —
the lock *was* the data it guarded, the same shape as #4203.

It had exactly one production consumer
(`OAuth2CredentialCollection.Load`, a get-or-add with `ParseExtension`
called inside the lock), so it could not simply be deleted. `Properties`
is now a `ConcurrentDictionary` — same public type, every operation
atomic on its own — and the one multi-operation need gets a member of
its own:

```csharp
public T GetOrAddProperty<T>(string key, Func<T> valueFactory);
```

It deliberately does **not** invoke the factory under a lock, so
`ParseExtension` now runs outside any critical section. Holding a lock
across a caller-supplied callback is exactly the shape that produced the
attribute/diagnostics deadlock earlier in this work.

The two existing tests asserted only `PropertiesLock is not null`, so
they tested nothing but its presence. They are replaced by tests of the
contract: factory runs once for a missing key, not at all for a present
one, 32 concurrent callers all observe the same instance, null arguments
throw.

`PropertiesLock` ships in 1.5.378, so this is a breaking removal —
migration guide section plus analyzer **UA0028**.

**`SharedDefinitionCache` no longer hands out its lock.** Three sites in
`SharedKeyValueSubscriptionStore` took the nested cache's `Lock` to
operate on the cache's own dictionary. Both are private now, behind
`ReplaceAll` and `CloneWhere`.

`SnapshotCommitLock` stays exposed on purpose: it is a `SemaphoreSlim`
sequencing an async commit across `await` points, which
`System.Threading.Lock` cannot span, so folding it in would change
behaviour rather than hide a lock.
## Verification

Merged `master` (`0edfe237a`, including #4234) with no conflicts.

- `dotnet build UA.slnx` — 0 warnings, 0 errors
- `Opc.Ua.Server.Tests` — 4074 passed
- `Opc.Ua.Types.Tests` — 8501 passed
- `Opc.Ua.MigrationAnalyzer.Tests` — 151 passed on all five TFMs (138 →
151, the new ones being UA0027 and UA0028)
- `Opc.Ua.Core.Tests` — 4249 passed
- `Opc.Ua.Redundancy.Server.Tests` — 573 passed

## Note for the reviewer

@romanett — your approval was given when this PR's diff was the 7
diagnostics files, because it was measured against
`marcschier/node-browse-locks`. Now that the two are combined and the
base is `master`, the diff is 23 files, and the extra 16 are the
node/browser lock work that was #4221. GitHub kept the approval across
the retarget, but it did not cover that code, so please re-review rather
than let it ride.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1
Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63
Copilot-Session: 0ee22c1d-c319-4257-bf78-a5a077a1824a
2026-08-11 22:14:10 +02:00

195 lines
6.0 KiB
C#

/* ========================================================================
* Copyright (c) 2005-2025 The OPC Foundation, Inc. All rights reserved.
*
* OPC Foundation MIT License 1.00
*
* Permission is hereby granted, free of charge, to any person
* obtaining a copy of this software and associated documentation
* files (the "Software"), to deal in the Software without
* restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following
* conditions:
*
* The above copyright notice and this permission notice shall be
* included in all copies or substantial portions of the Software.
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
* OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
* NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
* HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
* WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
* OTHER DEALINGS IN THE SOFTWARE.
*
* The complete license agreement can be found here:
* http://opcfoundation.org/License/MIT/1.00/
* ======================================================================*/
using System;
using System.Collections.Generic;
using Opc.Ua;
namespace MemoryBuffer
{
/// <summary>
/// A class to browse the references for a memory buffer.
/// </summary>
public class MemoryBufferBrowser : NodeBrowser
{
/// <summary>
/// Creates a new browser object with a set of filters.
/// </summary>
public MemoryBufferBrowser(
ISystemContext context,
ViewDescription view,
NodeId referenceType,
bool includeSubtypes,
BrowseDirection browseDirection,
QualifiedName browseName,
IEnumerable<IReference> additionalReferences,
bool internalOnly,
MemoryBufferState buffer)
: base(
context,
view,
referenceType,
includeSubtypes,
browseDirection,
browseName,
additionalReferences,
internalOnly)
{
m_buffer = buffer;
m_stage = Stage.Begin;
}
/// <summary>
/// Returns the next reference.
/// </summary>
public override IReference? Next()
{
// enumerate pre-defined references.
// always call first to ensure any pushed-back references are returned first.
IReference? reference = base.Next();
if (reference != null)
{
return reference;
}
if (m_stage == Stage.Begin)
{
m_stage = Stage.Components;
m_position = 0;
}
// don't start browsing huge number of references when only internal references are requested.
if (InternalOnly)
{
return null;
}
// enumerate components.
if (m_stage == Stage.Components)
{
if (IsRequired(ReferenceTypeIds.HasComponent, false))
{
reference = NextChild();
if (reference != null)
{
return reference;
}
}
m_stage = Stage.ModelParents;
m_position = 0;
}
// all done.
return null;
}
/// <summary>
/// Returns the next child.
/// </summary>
private NodeStateReference? NextChild()
{
MemoryTagState? tag;
// check if a specific browse name is requested.
if (!BrowseName.IsNull)
{
// check if match found previously.
if (m_position == uint.MaxValue)
{
return null;
}
// browse name must be qualified by the correct namespace.
if (m_buffer.TypeDefinitionId.NamespaceIndex != BrowseName.NamespaceIndex)
{
return null;
}
string? name = BrowseName.Name;
for (int ii = 0; ii < name!.Length; ii++)
{
if (!"0123456789ABCDEF".Contains(name[ii], StringComparison.Ordinal))
{
return null;
}
}
m_position = Convert.ToUInt32(name, 16);
// check for memory overflow.
if (m_position >= m_buffer!.SizeInBytes!.Value)
{
return null;
}
tag = new MemoryTagState(m_buffer, m_position);
m_position = uint.MaxValue;
}
// return the child at the next position.
else
{
if (m_position >= m_buffer!.SizeInBytes!.Value)
{
return null;
}
tag = new MemoryTagState(m_buffer, m_position);
m_position += m_buffer.ElementSize;
// check for memory overflow.
if (m_position >= m_buffer.SizeInBytes.Value)
{
return null;
}
}
return new NodeStateReference(ReferenceTypeIds.HasComponent, false, tag);
}
/// <summary>
/// The stages available in a browse operation.
/// </summary>
private enum Stage
{
Begin,
Components,
ModelParents,
Done
}
private Stage m_stage;
private uint m_position;
private readonly MemoryBufferState m_buffer;
}
}