GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-21 17:16:16 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,844 Commits 110 Branches 105 Tags
master
Commit Graph

3 Commits

Author SHA1 Message Date
Marc Schier 5899354441 Clarify documentation, migration guidance, and SDK selection (#4823) 
# 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>
 2026-10-21 10:10:55 +02:00
Copilot d525bc3f28 Bound reconnect cleanup and organize samples by domain (#4276) 
# Description

This PR fixes bounded ManagedSession reconnect cleanup and reorganizes
the sample applications into domain-focused folders.

## Reconnect cleanup

- Bounds reconnect integration cleanup with independent 30-second close
and dispose limits.
- Sends `CloseSession` through the awaited transport request path during
`Session.DisposeAsync`.
- Preserves explicit `Session.CloseAsync` behavior that waits for or
cancels outstanding publish requests.
- Covers the disposal request path with a regression test.

## Sample organization

- `samples/OpenUsd`: `GeneratorServer`, `SiteCompositionServer`
- `samples/Redundancy`: `RedundantClient`, `RedundantServer`,
`RedundantPubSub`
- `samples/Isa95`: `MinimalIsa95Server`
- `samples/MinimalApi`: `MinimalBoilerServer`, `MinimalCalcServer`,
`MinimalClient`
- `samples/DI`: `PumpDeviceIntegrationServer`
- `samples/Lds`: `ConsoleLdsServer`
- `samples/Reference`: `ConsoleReferenceClient`,
`ConsoleReferenceServer`
- `samples/PubSub`: `ConsoleReferencePubSubClient`

`UA.slnx` now exposes the same requested domain groups. Project
references, tests, runtime sample probes, CI workflows, Dockerfiles,
Compose files, helper scripts, and documentation were updated to the new
paths. `samples/Robotics/MinimalRobotServer` remains under `Robotics`.

## Validation

- `dotnet build UA.slnx`
- 3 Redundancy short-haul sample tests
- 70 DI sample tests
- 914 OpenUSD tests
- 1 WotCon model-materialization test
- 137 AOT tests
- `docker compose config` for all 6 moved Redundancy Compose files

## Related Issues

- Fixes #4273

## 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.
- [x] I have verified that my changes do not introduce new build or
analyzer warnings.
- [x] I built the complete `UA.slnx` solution locally.
- [ ] I ran all tests locally against at least .NET Framework and .NET
10.
- [ ] I fixed all failing and flaky tests in the CI pipelines and all
CodeQL warnings.
- [x] I have addressed all PR feedback received.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: Marc Schier <mschier@microsoft.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
Copilot-Session: a2c2838f-74ae-4997-a1d4-50574d9e0636
 2026-08-21 04:35:47 +00:00
Marc Schier e05a820c91 Removal of the exposed diagnostics and node locks (#4188) 
## What this is

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

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

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

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

## The analyses

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

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

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

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

## The design exploration

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

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

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

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

## Implementation: NodeState now guards its own attributes

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

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

### No lock is exposed anywhere, to anyone

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

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

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

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

### Migration coverage for the removed lock members

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

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

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

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

### The tests earned their keep

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

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

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

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

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

## Two corrections to my own published figures

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

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

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

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

## Caveats stated in the documents

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

## Review guidance

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

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

---

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

---------

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