GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-21 09:06:37 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,739 Commits 110 Branches 105 Tags
fix/encoder-stack-depth-json-flake
Commit Graph

3 Commits

Author SHA1 Message Date
romanett 28f1401324 Server: cut request-path lock contention and allocation; add load harness (#4707) 
Profiling the reference server under load showed most request-path CPU
spent spinning on contended locks, plus several avoidable allocations
per request or per node. This PR removes them, switches the reference
server to Server GC, and adds the load harness used to measure it.

## Results

Paired A/B against `master` (3 alternating rounds, medians, 20 sessions,
workstation GC on both sides, 24-core machine):

| Scenario | Throughput | p50 | p90 | Server CPU / op | Server alloc /
op | Lock contention |
|---|---|---|---|---|---|---|
| Read, 1 value | **+45%** (94k → 137k req/s) | −20% | −46% | −37% |
−39% | −94% |
| Read, 100 values | **+40%** | −26% | −17% | −24% | −25% | +130% ¹ |
| Read, 100 values, SignAndEncrypt | **+48%** | −36% | −20% | −30% |
−26% | +106% ¹ |
| Browse (Server object) | **+33%** | −31% | −12% | −13% | −14% | −40% |
| Write, 100 values | +13% | −12% | −4% | −10% | −19% | −23% |
| Data-change subscriptions | ±0 ² | ≈ | ≈ | −14% | −17% | −29% |

¹ The faster server spends more time in the remaining per-node locks.
² Throughput is bounded by the writer rate in this scenario.

Separately, Server GC instead of workstation GC doubles Read-100
throughput (21k → 44k req/s) and cuts p90 from 2.1 ms to 0.6 ms, because
workstation GC paused the server for about half the wall time under
load. The working set grows from ~210 MB to ~430 MB.

## Changes

- **`ObjectPool<T>`**: lock-free slot array. `ConcurrentBag.Count` took
every per-thread lock on each `Return` of the pooled response value-task
source, once per request.
- **`FairRequestQueue`**:
- drops redundant gate entries: the fail-fast pre-checks and the stopped
check read volatile fields and are re-checked under the gate; the owner
count is folded into selection;
- coalesces wake pulses with a pending flag, so the bounded signal
channel's lock is only taken when no token is outstanding. Wake-count
semantics in the tests are unchanged.
- **`DefaultServerResourceIsolationProvider`**:
  - compares peer addresses without `GetAddressBytes()` allocations;
- builds the unsecured verification context only when a classifier is
configured.
- **TCP listener / channel**: caches the certificate DER copies per
channel, keyed on the certificate instance. `X509Certificate2.RawData`
returns a fresh copy on every access, and the listener read it for every
request.
- **`EndpointBase`**: resolves the request `ActivitySource` once instead
of `Assembly.GetCallingAssembly()` per request.
- **Node managers**:
- one `ServerSystemContext` copy per request (cached on the
`OperationContext`) for per-node callbacks: permission metadata, node
metadata, view membership, method lookup;
  - removes a discarded copy in `GetReferenceDescriptionAsync`;
- per-request operation caches are plain `Dictionary` instead of a
`ConcurrentDictionary` with 24 lock objects;
  - the validation cache is built in one pass;
  - fixes zero-capacity error lists.
- **`ResourceManager`**: translation lookups read an immutable snapshot
that is rebuilt after writes. Browse serialised on the global lock for
every display name.
- **`MonitoredItem`**: shares the default `DataChangeFilter` instead of
allocating one per sampled value.
- **Reference server**: `ServerGarbageCollection=true`; new "Garbage
collector mode" section in `docs/ServerScalability.md`.
- **`perf/`**: `ServerLoadHarness` (out-of-process server plus load
driver, server-side CPU/GC/alloc/contention deltas, optional
`dotnet-trace` capture), `TraceAnalyzer` (TraceEvent-based
CPU/allocation/contention aggregation), and paired A/B scripts. Not part
of `UA.slnx`.

### Behaviour notes for custom node managers
- The `IDictionary<NodeId, NodeState>` operation cache handed to
overrides is now a `Dictionary`. It was a `ConcurrentDictionary`, but
only one request flow uses it.
- Within one request, per-node callbacks of a node manager share one
`ServerSystemContext` copy.

### Later commits on this PR
- **Permission checks without `NodeMetadata`** (`9068684c3`): the
role-permission and access-restriction validators run on an internal
`PermissionMetadata` struct; `AsyncCustomNodeManager` fills it on the
stack from the same attributes and namespace defaults. Browse's
reference filter reads 3 attributes per target instead of 13. Read 100
values: 120.6 → 95.7 KB/request, ~21k → 26k req/s; Browse 67.6 → 51.8
KB, 26.7k → 29.8k req/s.
- **Read-only channel evidence** (`936b28d76`, `dc638ab9f`):
`SecureChannelContext.ClientChannelCertificate`,
`ServerChannelCertificate` and `ChannelThumbprint` are `ByteString`, so
the cached per-channel certificate buffer is shared without a copy and
cannot be altered by handlers or classifiers. **Public API change**, see
the new section in `docs/MigrationGuide.md`. The isolation provider's
revalidation snapshot still copies.
- **Session lookup** (`936b28d76`):
`SessionManager.TryGetSessionContext` takes the candidate snapshot
without the bindings lock; the validating check under the lock is
unchanged (2 instead of 4 global lock acquisitions per request).
- Unit tests for `ObjectPool`, the per-request system context cache, the
translation snapshot, `SecureChannelContext` null semantics and a
stopped `FairRequestQueue`; `perf/` follows the repository conventions.

## Testing
- `Opc.Ua.Server.Tests` (net10.0): 8386 passed before the `ByteString`
change. After it, 13 secure-session and isolation tests failed; both
causes are fixed in `dc638ab9f` and those tests (25 with their variants)
pass. A full rerun of the Server, Sessions, Core, Redundancy, LDS and
GDS suites on the final commit is in progress.
- Full solution builds for net10.0 and net48; `dotnet format whitespace`
clean.

### o6-automation opcua-benchmarks (sampling, deep pipelines)
Replicated both tests of the [o6-automation
benchmark](https://github.com/o6-automation/opcua-benchmarks) in the
harness (`--scenario counters`, and `read` with 10 clients × N
outstanding), paired against `origin/master`:

| Test | master | this PR |
|---|---|---|
| Subscription sampling, 100 Hz, every item ≥ 99 % of samples, 1 item |
FAIL (75–94 %) | PASS (100 %) |
| same, 1024 items | FAIL (64–73 %) | PASS (99.9 %) |
| same, capacity | — | between 4096 and 6144 items (~410–520k samples/s)
|
| Read, 10 clients × 100 outstanding, None | stalls (0–1.7k req/s) |
121–131k req/s |
| Read, 10 clients × 100 outstanding, SignAndEncrypt | stalls (0 req/s)
| 89–91k req/s |
| Read, 10 clients × 1000 outstanding | channels faulted
(`BadTcpNotEnoughResources`) | 0 errors, ~45–54k req/s |

- **Fixed-rate sampling** (`30b769d13`): the sampling loop waited one
interval after each sample, adding timer overshoot and sampling time to
every cycle; samples now have absolute deadlines.
- **Async response send** (`cdac5c5ca`): `SendResponse` blocked
thread-pool workers on the channel gate; with many requests in flight
the receive loop that was handed the gate waited for a pool thread and
the channel stalled. The TCP, Kestrel TCP and WebSocket listeners now
use `SendResponseAsync`.
- **Backpressure instead of channel fault** (`191cd2ba7`): server
channels stop reading while more than 100 responses are queued and
resume at 50, instead of faulting the channel. Client channels keep
reading. At 10,000 outstanding requests throughput is ~50k req/s with or
without the limit engaged, so the remaining gap to the 100-per-client
case is not the backpressure.

## Not changed
- `SentMessageQueue` deep-clones every `NotificationMessage`. Retained
messages return their pooled payloads on acknowledgement while the
response copy may still be queued for send, and a client can acknowledge
a sequence number before receiving it; the clone keeps a recycled
payload from reaching another session's response. Removing it needs a
send-completion hook from the transport.
- Generated responses allocate a default `ResponseHeader` that is
replaced (~2-3 % of per-request allocation). Fixing it means making
structure fields of every generated type lazily initialized, a
generator-wide change to default values, equality and encoding.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-09 13:00:49 +02:00
romanett 9b034f8291 Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) 
# Description

Nullable reference types are now enabled once, in `common.props`, and
every project in the repository builds warning-clean with them.

**Configuration**
- `<Nullable>enable</Nullable>` lives in `common.props`. All per-project
`<Nullable>` settings are removed: 178 project files (enable,
annotations, and the `disable` in the three Aot test projects).
- 419 per-file `#nullable enable`/`restore` directives and the 4
`#nullable disable` directives are removed.
- Kept on purpose:
- The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type
Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`,
`PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the
project setting in generated code, so the directive is required there.
- `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against
the 1.5.378 packages and does not import the repository props.
- `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now
describe the central setting.

**Migration** (the 43 unset and 13 annotations-only projects, roughly
10k diagnostics)
- **Production code: annotated at the declarations.** Covers
`tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the
LDS sample.
- Members, parameters and returns are nullable where null actually
flows.
- Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`,
`IsMethodTypeDesign`).
- `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take
`params object?[]`. `Template.AddReplacement` takes `string?` (rendering
already skips null replacements).
- `IRegisteredServerStore.RegisterAsync` returns
`Task<RegistrationEntry?>`: it returns null when a registration is
removed.
- Dereferences backed by an invariant (for example, the validated model)
use `!`, so runtime behaviour is unchanged.
- **Behavioural changes in production code** (small, deliberate):
- `NodeSetToModelDesign` throws `InvalidOperationException` when the
file is not a NodeSet. Before, it threw a NullReferenceException on the
next line.
- `ModelDesignValidator` starts its lookup tables empty instead of null.
A redundant `if (m_nodes == null) return;` guard is gone.
- **Test code: minimal annotations.**
  - `?` on values that can be null, `!` where the test asserts non-null.
  - `= null!` for fixture members assigned in SetUp.
- `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the
`IEncodeable` constraint.
- A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`)
folded into the adjacent `IsNull`/`Count` checks.

**Notes for reviewers**
- The diff is large (~1,500 files) but almost entirely mechanical: `?`,
`!` and directive removal. The non-annotation changes are listed above.
- Known follow-up, not done here:
`IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay
non-nullable even though the validator returns null before it loads a
dictionary. Making them nullable would ripple through every generator
call site.

## Related Issues

- Fixes #4439

## Checklist

- [ ] 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.
- [x] 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.

The "no new warnings" box covers net10.0 and net48: full non-incremental
builds of `UA.slnx`, `tools/SourceGeneration.slnx`,
`tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0
warnings and 0 errors under the repository's warnings-as-errors
settings. net9.0 and net8.0 also had no nullable diagnostics, but they
were not rebuilt after the final fixes. Tests have not been run locally
yet.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-08 08:48:13 +02:00
romanett efad0ddeff Server subscriptions: fix spec defects in Subscription/MonitoredItem handling found by audit (#4629) 
# Description

Fixes defects found in a spec-grounded audit of the server
**Subscription / MonitoredItem** implementation
(`src/Opc.Ua.Server/Subscription/**`, `NodeManager/MonitoredItem/**`,
`EventManager`, monitored-item service dispatch and filter validation).
Each behaviour change was checked against OPC 10000-4 (§5.13, §5.14,
§7.21, §7.22), Part 5 (subscription diagnostics) and Part 9
(ConditionRefresh). Every fix has a regression test.

The audit found 56 defects (1 Critical, 3 High, 20 Medium, 32 Low). A
follow-up max-effort code review of the fix branch found 27 more issues,
mostly introduced by the first round of fixes. All but one are fixed
here.

### Most important fixes
- **Critical — remote hang:** an `AggregateFilter` with
`processingInterval = NaN` was not revised, and `AggregateCalculator`
looped forever while holding the node-manager lock, so one
CreateMonitoredItems request blocked the node manager. Non-finite, ≤0
and sub-tick intervals are now revised (Part 4 §5.13.2.1), and the
calculator rejects them as a backstop.
- **Monitored-item limits:** new
`ServerConfiguration.MaxMonitoredItemCount` /
`MaxMonitoredItemsPerSubscription`; Bad_TooManyMonitoredItems is
returned when a limit is hit, and ServerCapabilities now report the
limits. Both default to `0` (no limit), so existing behaviour is
unchanged.
- **Sampling groups:** a client could get one dedicated thread per
distinct sampling interval above 1 h; intervals above the last rate
group are now rounded, and the loop is async. An item could also end up
in two sampling groups and keep being sampled after it was deleted.
- **Subscription state machine** (Part 4 §5.14.1.2):
- the lifetime counter resets while a Publish is queued and counts
otherwise;
  - the lifetime and keep-alive stay within `MaxSubscriptionLifetime`;
  - LatePublishRequestCount is counted once per entry into LATE;
- messages held while publishing is disabled are no longer sent, and are
not offered by Transfer/Republish either;
- ConditionRefresh returns Bad_RefreshInProgress for any refresh already
queued or running on the subscription.
- **Triggering** (Part 4 §5.13.1.6):
- linked items are triggered by an item still draining a backlog, by the
first value after re-enable, and by an item created Disabled;
  - links are persisted for durable subscriptions;
- Disabled/Sampling items follow the publish rules in the shared
pipeline, so custom `IMonitoredItem` implementations follow them too.
- **Queues** (Part 4 §5.13.1.5, §7.21):
  - no duplicate value after a queue-size increase;
- EventQueueOverflow events are placed where the events were lost,
including on queue shrink;
  - the duplicate-event check scans the newest entries;
- EventQueueOverflowCount and MonitoringQueueOverflowCount are
maintained correctly;
- changing select clauses rebuilds queued events in the new layout, and
is safe on durable queues and on failure.
- **Filters:**
- per-clause EventFilterResult instead of rejecting the whole item (Part
4 §7.22.3);
- corrected status codes: Bad_DeadbandFilterInvalid,
Bad_MonitoredItemFilterInvalid, Bad_EventFilterInvalid,
Bad_MonitoringModeInvalid, Bad_TimestampsToReturnInvalid;
- aggregate filter in subscriptions: processing interval at least 2× the
revised sampling interval, revised StartTime on the requested grid,
ServerTimestamp = end of interval (Part 4 §7.22.4).
- **Lifecycle and races:**
- expired subscriptions are always cleaned up, even when the session
closes during cleanup;
  - Create/Transfer racing CloseSession returns Bad_SessionClosed;
  - session diagnostics counts no longer drift;
  - a large but valid Publish TimeoutHint no longer fails the request;
- a ready subscription is served by queued Publish requests instead of
going Late;
  - restored persisted state is validated.
- **Access:** a value change notification is checked against each
subscriber's UserAccessLevel, not the writer's.
- **Redundancy / durable store:**
  - the first-unsent sequence number is mirrored;
  - sequence numbers are no longer reused after a second failover;
- triggering links are stored (definition format 6, written only when
links exist, so older replicas can still read link-less subscriptions).

### Behaviour changes reviewers should know
- With the default config, publishing intervals above
`MaxSubscriptionLifetime / 3` (1,200,000 ms) are revised down, so
lifetime ≥ 3×keep-alive fits the configured maximum.
- The revised keep-alive is capped at a third of the max lifetime.
- Idle subscriptions expire after exactly MaxLifetimeCount cycles
without a queued Publish.
- A partly invalid EventFilter select list is accepted with per-clause
errors. The public `EventFilter.Validate` keeps Status Good if at least
one clause is valid; callers that cannot return per-clause results must
also check the new `EventFilter.Result.HasSelectClauseErrors`. There is
a migration note in `docs/migrate/2.0.x/sessions-subscriptions.md`.
- Aggregate processing intervals may be revised higher (≥ 2× sampling
interval); aggregate ServerTimestamp is now the interval end.
- Sampling intervals above 3,600,000 ms are rounded to multiples of
900,000 ms (default rate groups).

### Public API
Additive only:
- `ServerConfiguration.MaxMonitoredItemCount` and
`MaxMonitoredItemsPerSubscription`;
- `EventFilter.Result.HasSelectClauseErrors`;
- `IStoredSubscriptionTriggering`,
`ISubscriptionRetransmissionSendStateStore`,
`SubscriptionRetransmissionState.FirstUnsentSequenceNumber` (optional
store capabilities, detected with `as`);
- new `EventQueueHandler` constructor overloads.

### Known open items (not in this PR)
- **Part 4 §5.13.2.1:** CreateMonitoredItems should create an item even
when the user lacks Read permission and report the error in Publish.
This is not done: the create-time RolePermissions check is the only role
check on the initial value, so following the spec would leak values
until every node manager re-checks per sample.
- **Sampling loop:** it runs on thread-pool threads; node managers with
blocking synchronous reads should not use sampling groups heavily. The
alternative, a dedicated thread per group, was the thread-exhaustion
issue fixed here.
- **Restarts:** a pending QueueSize-1 value and a pending overflow flag
are not kept across a restart; that needs persistence interface
extensions.
- **Limit defaults:** decide whether the new limits should get finite
defaults (currently 0 = unlimited).

## Related Issues

- No tracking issue; the findings come from an internal audit of the
server subscription code.

## Checklist

- [ ] 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.
- [x] 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.

### Local verification (net10.0, all green)
| Suite | Passed | Failed |
|---|---|---|
| Opc.Ua.Server.Tests | 7963 | 0 |
| Opc.Ua.Subscriptions.Tests | 643 | 0 |
| Opc.Ua.Subscriptions.Classic.Tests | 57 | 0 |
| Opc.Ua.Subscriptions.Durable.Tests | 36 | 0 |
| Opc.Ua.Redundancy.Server.Tests | 847 | 0 |
| Opc.Ua.Core.Tests (filter tests) | 283 | 0 |

net48 build of `Opc.Ua.Server.Tests`: 0 errors. net48 tests and the full
UA.slnx were not run locally.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
 2026-10-03 05:43:16 +02:00