Files
Marc Schier 5bc8a9d705 Reduce ISession: type the continuation points, delete the obsolete validate overload (#4199)
## Stacked on #4197

Base is `marcschier/server-bind-phase`. The full stack is **#4188 →
#4193 → #4197 → this**.
GitHub retargets each base automatically as the stack merges. **Review
the earlier three
first.**

**#4217 has been merged into this branch** (squash `53dd4b618`), so the
last remaining
item from the issue — `ISession.SessionDiagnostics` — is now part of
this PR.

Closes #4187.

Refs #4203 — **does not close it.** #4217 swapped `OnUpdateDiagnostics`
onto
`m_diagnosticsLock`, but it still calls
`Variant.FromStructure(ServerDiagnostics)` with the
default `copy: false`, which wraps the *live* structure in the
`ExtensionObject`. The caller
reads its fields after the lock is released, so the lock currently
excludes nothing. #4222
adds `copy: true` and is what actually closes #4203.

## The finding that changed the approach

`ISession` has **exactly one production adapter**:
`src/Opc.Ua.Server/Session/Session.cs`.
The other `ISession` implementations in the tree are the unrelated
**client** interface
(`src/Opc.Ua.Client/Session/Session.cs`, `RedundantClientSession`) and a
migration-analyzer test stub.

One adapter means the seam is hypothetical; two means it is real. The
issue proposed
splitting `ISession` into five concern-interfaces — that would turn one
hypothetical seam
into five. The interface earns its keep for testability and because node
managers receive
it through `OperationContext`, but that argues for making it
**smaller**, not for
multiplying it.

**So: reduce, don't split.** Reasoning posted to the issue as
[comment
5226712936](https://github.com/OPCFoundation/UA-.NETStandard/issues/4187#issuecomment-5226712936)
so the five-way framing does not get re-proposed later.

## Measurement

38 members, 368 references. External production consumers are **4 files
using 5 members**:

| Consumer | Members used |
|---|---|
| `Opc.Ua.Redundancy.Server/Sessions/DistributedSessionManager.cs` |
`Id`, `InitializeAsync`, `Dispose` |
| `Opc.Ua.Di.Server/Locking/DefaultLockService.cs` | `Id` |
| `samples/ConsoleReferenceServer/UAServer.cs` | `Id`, `Identity`,
`ReadDiagnostics` |
| `samples/Quickstarts.Servers/.../DataChangeMonitoredItem.cs` | type
reference only |

Every production caller of the five continuation-point members is inside
`Opc.Ua.Server`
(`MasterNodeManager`, `NodeManagerLifecycle`, `HistorianDispatcher`).

**`ISession` is 38 → 32 members.**

## Commits

### `882b2937` Type the history continuation point

`ISession` leaked `object` twice, against the repository rule:

```csharp
void SaveHistoryContinuationPoint(Guid id, object continuationPoint);
object? RestoreHistoryContinuationPoint(ByteString continuationPoint);
```

The erasure was **not** hiding a genuinely opaque plugin token.
`HistorianDispatcher` is the
only production caller, it always passes a `HistorianContinuationState`,
and it casts
straight back to that type on restore. The `object` was a round trip
through the type system
that bought nothing.

`HistorianContinuationState` is `internal`, so it cannot appear on a
public interface.
`IHistoryContinuationPoint` names the concept instead: it carries an
`Id` and extends
`IDisposable`.

Two latent bugs fell out of typing it:

- The session disposed only those points that **happened** to implement
`IDisposable` and
**silently leaked the rest**. Every point is now disposable by
construction, and three
  `as IDisposable` probes are gone.
- Carrying the `Id` on the point removes the separate `Guid` parameter,
which could disagree
with the payload it was filed under. The mock stores in the historian
tests had the same
  latent inconsistency.

### `0d020b77` Delete the obsolete synchronous `ValidateBeforeActivate`

**Zero production callers** — the nine references were all tests written
for it. `[Obsolete]`
since 1.5.378, which the repository rules allow removing in master.

It is also the divergent-signature problem the runtime analysis flagged:
the async form
returns the validated handler and policy as a tuple, while the sync one
took the same two
values as trailing `out` parameters, so a caller had to learn two shapes
for one operation.

The overload could not verify a user token requiring decryption, so on a
secure endpoint it
failed closed and told the caller to use the async path anyway. The one
test asserting that
fail-closed behaviour goes with the method it described; the path it
pointed at is already
covered by `ActivationValidationDecodesBinaryEncodedUserIdentityToken`.

The other eight tests are **converted, not deleted** — the channel,
signature,
security-policy and token-policy validation they cover is still
asserted, now against the
path the server actually uses.

### `821e8302` Route the continuation points behind one member

Five `ISession` members were one-line pass-throughs to
`SessionContinuationPoints`, a
419-line module that already owns the behaviour and already has its own
`IContinuationPointStore` seam. The interface restated the module's
surface without adding
anything.

```csharp
// was
session.SaveContinuationPoint(cp);
session.RestoreHistoryContinuationPoint(bytes);

// now
session.ContinuationPoints.SaveBrowse(cp);
session.ContinuationPoints.RestoreHistory(bytes);
```

The holder is `internal sealed`, so `ISessionContinuationPoints` is what
makes it
addressable.

**This is composition, not the service locator `IServerContext` refused
to be.** A subsystem
with an independent lifetime should be injected; continuation points
belong to the session
and cannot exist without it.

`InvalidateContinuationPoints` called `RemoveBrowseForManager` and
`RemoveHistoryForManager`
in sequence, so the pair becomes `RemoveForManager` — the caller's
intent is that a node
manager is going away, not that two collections need visiting.

## Tests

Three harnesses had hand-rolled a `Dictionary<Guid, object>` to stand in
for the holder. They
now use the **real** one, so the dispatcher and `BrowseNext` meet the
eviction and disposal
rules a live session actually applies.

**One hazard worth recording for future interface work.** A Moq member
that is never stubbed
returns `null`. Where the old code called a method straight on
`ISession`, that `null` was
harmless. Reaching *through* a property means an unstubbed
`ContinuationPoints` null-derefs
instead. Three tests failed exactly that way; they are wired to a real
holder, not restubbed.

| Suite | net10.0 | net48 |
|---|---|---|
| `Opc.Ua.Server.Tests` | 4032 passed, 1 failed | builds clean |
| Touched areas (continuation points, historian, master node manager,
session) | 898 passed | — |

The single failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a
local certificate-store problem verified to reproduce unchanged at HEAD
with these changes
stashed. Not caused by this PR.

`Opc.Ua.Redundancy.Server`, `Opc.Ua.Di.Server`, `Quickstarts.Servers`
and
`ConsoleReferenceServer` all build clean.

## Docs

`docs/MigrationGuide.md` gains a "Removed members on ISession" section
covering the
`ValidateBeforeActivate` removal with the async replacement, and the
`IHistoryContinuationPoint` change with before/after.

## The SessionDiagnostics removal (folded in from #4217)

`ISession.SessionDiagnostics` handed out the whole mutable
`SessionDiagnosticsDataType` —
the structure the session's diagnostics lock protects — so a caller
could read a field while
the owner was writing it. Every server-side reader wanted exactly one
value out of it, so the
interface now exposes those two values (`SessionName`,
`ClientApplicationUri`) directly and
the structure stays behind the lock.

That work landed here as #4217 rather than as a separate PR, and it also
fixes #4203:
`OnUpdateDiagnostics` now takes `m_diagnosticsLock` instead of locking
the diagnostics
payload it was handed.

## Reviewer questions

- `ISessionContinuationPoints` exposes `MaxBrowse` as read-only while
the concrete holder
keeps a setter (the test framework adjusts it to exercise limits).
Should the setter be on
the interface, or is test-only mutation better left to the concrete
type?
- `RemoveForManager` collapses two removal methods. The concrete
`RemoveBrowseForManager`/`RemoveHistoryForManager` remain public on the
holder — worth
  making them private now that nothing outside calls them individually?

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
2026-08-11 06:57:40 +02:00

419 lines
16 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 System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Opc.Ua.Security.Certificates;
using Opc.Ua.Server;
using Quickstarts.ReferenceServer;
namespace Opc.Ua.Client.TestFramework
{
/// <summary>
/// <para>
/// Reference Server modification which allows to change the maximum number
/// of (browse) continuation points during the execution of a test.
/// This makes it easier to compare browse results without any restriction
/// with results where the server may allocate only a limited number of
/// continuation points.
/// To make this work some other classes must be derived and modified, as
/// well (see the ServerSessionWithLimits, SessionManagerWithLimits and
/// MasterNodeManagerWithLimits classes).
/// </para>
/// <para>
/// Use with care. This class and especially its dedicated functionality
/// should be used for test purposes only.
/// </para>
/// </summary>
public class ReferenceServerWithLimits : ReferenceServer
{
public ReferenceServerWithLimits(ITelemetryContext telemetry)
: base(telemetry)
{
}
public uint TestMaxBrowseReferencesPerNode { get; set; } = 10u;
private MasterNodeManager MasterNodeManagerReference { get; set; }
private SessionManagerWithLimits SessionManagerForTest { get; set; }
public override ValueTask<BrowseResponse> BrowseAsync(
SecureChannelContext secureChannelContext,
RequestHeader requestHeader,
ViewDescription view,
uint requestedMaxReferencesPerNode,
ArrayOf<BrowseDescription> nodesToBrowse,
RequestLifetime requestLifetime)
{
return base.BrowseAsync(
secureChannelContext,
requestHeader,
view,
TestMaxBrowseReferencesPerNode,
nodesToBrowse,
requestLifetime);
}
public void SetMaxNumberOfContinuationPoints(uint maxNumberOfContinuationPoints)
{
Configuration.ServerConfiguration.MaxBrowseContinuationPoints
= (int)maxNumberOfContinuationPoints;
((MasterNodeManagerWithLimits)MasterNodeManagerReference)
.MaxContinuationPointsPerBrowseForUnitTest =
maxNumberOfContinuationPoints;
foreach (Server.ISession session in SessionManagerForTest.GetSessions().ToList())
{
try
{
((ServerSessionWithLimits)session).SetMaxNumberOfContinuationPoints(
maxNumberOfContinuationPoints);
}
catch
{
}
}
}
protected override IMasterNodeManager CreateMasterNodeManager(
IServerInternal server,
ApplicationConfiguration configuration)
{
m_logger.LogInformation(
Utils.TraceMasks.StartStop,
"Creating the Reference Server Node Manager.");
var nodeManagers = new List<INodeManager>();
// create the custom node manager (ownership transfers to MasterNodeManager).
#pragma warning disable CA2000 // Dispose objects before losing scope - ownership transfers to MasterNodeManagerWithLimits
var referenceNodeManager = new ReferenceNodeManager(
server,
configuration);
#pragma warning restore CA2000
List<IAsyncNodeManager> asyncNodeManagers = [referenceNodeManager];
foreach (INodeManagerFactory nodeManagerFactory in NodeManagerFactories)
{
nodeManagers.Add(nodeManagerFactory.Create(server, configuration));
}
//this.MasterNodeManagerReference = new MasterNodeManager(server, configuration, null, nodeManagers.ToArray());
MasterNodeManagerReference = new MasterNodeManagerWithLimits(
server,
configuration,
null,
asyncNodeManagers,
[.. nodeManagers]);
// create master node manager.
return MasterNodeManagerReference;
}
protected override ISessionManager CreateSessionManager(
IServerInternal server,
ApplicationConfiguration configuration)
{
SessionManagerForTest = new SessionManagerWithLimits(server, configuration);
return SessionManagerForTest;
}
}
/// <summary>
/// provide a means to set the maximum number of browse continuation points to
/// the (Server) session.
/// </summary>
public class ServerSessionWithLimits : Server.Session
{
public ServerSessionWithLimits(
OperationContext context,
IServerInternal server,
Certificate serverCertificate,
NodeId authenticationToken,
ByteString clientNonce,
Nonce serverNonce,
string sessionName,
ApplicationDescription clientDescription,
string endpointUrl,
Certificate clientCertificate,
CertificateCollection clientCertificateChain,
double sessionTimeout,
uint maxResponseMessageSize,
double maxRequestAge,
int maxBrowseContinuationPoints,
int maxHistoryContinuationPoints)
: base(
context,
server,
serverCertificate,
authenticationToken,
clientNonce,
serverNonce,
sessionName,
clientDescription,
endpointUrl,
clientCertificate,
clientCertificateChain,
sessionTimeout,
maxBrowseContinuationPoints,
maxHistoryContinuationPoints)
{
}
public void SetMaxNumberOfContinuationPoints(uint maxNumberOfContinuationPoints)
{
MaxBrowseContinuationPoints = (int)maxNumberOfContinuationPoints;
}
}
/// <summary>
/// ensures that the (Server) session is the derived one for the test.
/// </summary>
public class SessionManagerWithLimits : SessionManager
{
private readonly IServerInternal m_4TestServer;
private readonly int m_4TestMaxRequestAge;
private readonly int m_4TestMaxBrowseContinuationPoints;
private readonly int m_4TestMaxHistoryContinuationPoints;
public SessionManagerWithLimits(
IServerInternal server,
ApplicationConfiguration configuration)
: base(server, configuration)
{
m_4TestServer = server;
m_4TestMaxRequestAge = configuration.ServerConfiguration.MaxRequestAge;
m_4TestMaxBrowseContinuationPoints = configuration.ServerConfiguration
.MaxBrowseContinuationPoints;
m_4TestMaxHistoryContinuationPoints = configuration.ServerConfiguration
.MaxHistoryContinuationPoints;
}
/// <summary>
/// TBD - Remove unused parameter.
/// </summary>
protected override Server.ISession CreateSession(
OperationContext context,
IServerInternal server,
Certificate serverCertificate,
NodeId sessionCookie,
ByteString clientNonce,
Nonce serverNonce,
string sessionName,
ApplicationDescription clientDescription,
string endpointUrl,
Certificate clientCertificate,
CertificateCollection clientCertificateChain,
double sessionTimeout,
uint maxResponseMessageSize,
int maxRequestAge, // TBD - Remove unused parameter.
int maxContinuationPoints)
{
return new ServerSessionWithLimits(
context,
m_4TestServer,
serverCertificate,
sessionCookie,
clientNonce,
serverNonce,
sessionName,
clientDescription,
endpointUrl,
clientCertificate,
clientCertificateChain,
sessionTimeout,
maxResponseMessageSize,
m_4TestMaxRequestAge,
m_4TestMaxBrowseContinuationPoints,
m_4TestMaxHistoryContinuationPoints);
}
}
/// <summary>
/// Hack which ensures the injected maximum number of browse continuation points
/// is really used when the browse service call is executed.
/// </summary>
public class MasterNodeManagerWithLimits : MasterNodeManager
{
public MasterNodeManagerWithLimits(
IServerInternal server,
ApplicationConfiguration configuration,
string dynamicNamespaceUri,
IEnumerable<IAsyncNodeManager> additionalManagers,
IEnumerable<INodeManager> additionalSyncManagers)
: base(server, configuration, dynamicNamespaceUri, additionalManagers, additionalSyncManagers)
{
m_logger = server.Telemetry.CreateLogger<MasterNodeManagerWithLimits>();
}
public uint MaxContinuationPointsPerBrowseForUnitTest { get; set; }
/// <summary>
/// Returns the set of references that meet the filter criteria.
/// </summary>
/// <exception cref="ArgumentNullException"><paramref name="context"/> is <c>null</c>.</exception>
/// <exception cref="ServiceResultException"></exception>
public override async ValueTask<(ArrayOf<BrowseResult> results, ArrayOf<DiagnosticInfo> diagnosticInfos)> BrowseAsync(
OperationContext context,
ViewDescription view,
uint maxReferencesPerNode,
ArrayOf<BrowseDescription> nodesToBrowse,
CancellationToken cancellationToken = default)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}
if (view != null && !view.ViewId.IsNull)
{
(object viewHandle, IAsyncNodeManager viewManager) =
await GetManagerHandleAsync(view.ViewId, cancellationToken)
.ConfigureAwait(false);
if (viewHandle == null || viewManager == null)
{
throw new ServiceResultException(StatusCodes.BadViewIdUnknown);
}
NodeMetadata metadata = await viewManager.GetNodeMetadataAsync(
context,
viewHandle,
BrowseResultMask.NodeClass,
cancellationToken)
.ConfigureAwait(false);
if (metadata == null || metadata.NodeClass != NodeClass.View)
{
throw new ServiceResultException(StatusCodes.BadViewIdUnknown);
}
// validate access rights and role permissions
ServiceResult validationResult = await ValidatePermissionsAsync(
context,
viewManager,
viewHandle,
PermissionType.Browse,
null,
true,
cancellationToken)
.ConfigureAwait(false);
if (ServiceResult.IsBad(validationResult))
{
throw new ServiceResultException(validationResult);
}
view.Handle = viewHandle;
}
bool diagnosticsExist = false;
var results = new List<BrowseResult>(nodesToBrowse.Count);
var diagnosticInfos = new List<DiagnosticInfo>(nodesToBrowse.Count);
uint continuationPointsAssigned = 0;
for (int ii = 0; ii < nodesToBrowse.Count; ii++)
{
// check if request has timed out or been cancelled.
if (StatusCode.IsBad(context.OperationStatus))
{
// release all allocated continuation points.
foreach (BrowseResult current in results)
{
if (current != null && !current.ContinuationPoint.IsEmpty)
{
ContinuationPoint cp = context.Session
.ContinuationPoints.RestoreBrowse(current.ContinuationPoint);
cp.Dispose();
}
}
throw new ServiceResultException(context.OperationStatus);
}
BrowseDescription nodeToBrowse = nodesToBrowse[ii];
// initialize result.
var result = new BrowseResult { StatusCode = StatusCodes.Good };
results.Add(result);
ServiceResult error;
// need to trap unexpected exceptions to handle bugs in the node managers.
try
{
error = await BrowseAsync(
context,
view,
maxReferencesPerNode,
MaxContinuationPointsPerBrowseForUnitTest <= 0 ||
continuationPointsAssigned < MaxContinuationPointsPerBrowseForUnitTest,
nodeToBrowse,
result,
cancellationToken).ConfigureAwait(false);
}
catch (Exception e)
{
error = ServiceResult.Create(
e,
StatusCodes.BadUnexpectedError,
"Unexpected error browsing node.");
}
// check for continuation point.
if (!result.ContinuationPoint.IsEmpty)
{
continuationPointsAssigned++;
}
// check for error.
result.StatusCode = error.StatusCode;
if ((context.DiagnosticsMask & DiagnosticsMasks.OperationAll) != 0)
{
DiagnosticInfo diagnosticInfo = null;
if (error != null && error.Code != StatusCodes.Good)
{
diagnosticInfo = ServerUtils.CreateDiagnosticInfo(Server, context, error, m_logger);
diagnosticsExist = true;
}
diagnosticInfos.Add(diagnosticInfo);
}
}
// clear the diagnostics array if no diagnostics requested or no errors occurred.
UpdateDiagnostics(context, diagnosticsExist, ref diagnosticInfos);
return (results, diagnosticInfos);
}
private readonly ILogger m_logger;
}
}