mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-18 14:43:08 +08:00
master
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
668310f21f |
Make the fluent node manager's configure/seal path genuinely async (#4442)
# Description The fluent node-manager pipeline had two synchronous chokepoints, and two independent efforts have now hit both of them. `Configure` is a `partial void`, so it cannot `await`. A manager whose wiring depends on asynchronous setup had to invent a second hook outside the pipeline, create its own builder there, and hand-roll `CreateFluentBuilder(...).Configure(...).Seal()` inside it. `Seal()` was synchronous, so nothing a registration staged during `Configure` could be completed by awaiting. Registrations that needed to await either blocked — `EventSourceRegistry` did `.GetAwaiter().GetResult()` on the manager's root-notifier registration — or could not be expressed at all. ## What changed **`FluentNodeManagerBase.ConfigureAsync(INodeManagerBuilder, CancellationToken)`** is the awaitable wiring seam. It runs once per activation, with the manager's attached builder, immediately *before* the synchronous `Configure` partial(s) — so instances it materialises are in the address space by the time `Configure` wires callbacks against them. The default implementation is a no-op. It is a `virtual` method rather than a second `partial` declaration because a partial method can be optional (`partial void`, no return value) or awaitable (an extended partial method, which must be implemented) — not both, and the generator would otherwise have to detect which one the user declared. `partial void Configure(INodeManagerBuilder)` keeps working unchanged; a manager uses either hook or both. **`NodeManagerBuilder.Seal()` becomes `SealAsync(CancellationToken)`.** Sealing is now the single point where registrations that could not finish inside `Configure` get their turn to await. Every seal site awaits it, so a later change can move registry *setup* onto the same seam. **`DiNodeManager` now runs the standard pipeline itself** — create and attach the builder, `await ConfigureAsync`, `RegisterAuthoredNodesAsync`, `CompleteConfigureAsync`, the post-setup runner, then `await builder.SealAsync` — instead of handing subclasses a bare callback and leaving them to build and seal on their own. DI subclasses gain node registration and the reverse-reference pass, which they previously skipped. Sealing deliberately comes **after** the post-setup runner. The fluent registries are owned by the manager, not by a single builder, and sealing starts them — so sealing first locks post-setup configurators out of registering simulation loops of their own, which is exactly what `AddRobotics`' configurators do. Sealing before the runner makes `MinimalRobotServer` fail to start with *"Cannot add a simulation loop after the registry has started"*. Worth knowing for follow-up work: with several builders attached to one manager, the first seal freezes the shared registry for all of them. ## Workarounds removed - **`DiNodeManager.OnAddressSpaceReadyAsync`** — deleted. Its three overriders (`RoboticsNodeManager`, and the `PumpNodeManager` / `GeneratorNodeManager` samples) now override `ConfigureAsync` and no longer hand-roll `CreateFluentBuilder(...).Configure(Configure).Seal()`. - **`EventSourceRegistry`'s `.GetAwaiter().GetResult()`** on `AddRootNotifierAsync` — gone. `Publish(..., RegisterAsRootNotifier: true)` now stages the registration and `SealAsync` drains it, so the wait happens on an awaited path. A source whose root-notifier registration fails is rolled out of the registry before the failure surfaces, and a registry disposed before its builder sealed drops what it staged. - **`Isa95NodeManager.ConfigureStatusEvents()`** — the synchronous second builder pass is now `ConfigureStatusEventsAsync(CancellationToken)`, awaited from `CreateAddressSpaceAsync`. (`ConfigureCatalogChanges()` stays synchronous: it starts a background task and never seals a builder, so there was nothing to convert.) - **`IRoboticsBuildContext.Seal()`** — now `SealAsync(CancellationToken)`. These really were `NodeManagerBuilder.Seal()` calls behind a same-named method. The state transition still happens under the context lock; the builder's asynchronous completion runs outside it, so no `await` is taken while the lock is held. - The Pump / Generator incremental registration callbacks (`onRegistered`) changed from `Action<T>` to `Func<T, CancellationToken, ValueTask>` so their per-instance builder pass can await its own seal. No sync-over-async was introduced anywhere. `Dispose(bool)` is unchanged and stays non-blocking; teardown remains in `DeleteAddressSpaceAsync`. `NodeManagerBuilder.Seal()` and `IRoboticsBuildContext.Seal()` are removed rather than kept as synchronous overloads. A synchronous seal that silently skipped the asynchronous completion would reintroduce exactly the failure mode this change exists to remove. ## Generator `NodeManagerTemplates` emits, in order: predefined nodes load → `await ConfigureAsync(__m_builder, cancellationToken)` → `Configure(__m_builder)` and the typed `Configure` → `RegisterAuthoredNodesAsync` → `CompleteConfigureAsync` → `await SealConfigurationAsync(__m_builder, cancellationToken)`. `NodeManagerGeneratorTests` asserts that order literally, and that neither `__m_builder.Seal()` nor the synchronous `SealConfiguration(__m_builder)` is emitted. ### Merging with #4432 #4432 landed on `master` while this was in review and reworked the same seam from the other side: it split `Seal()` into `SealGraphAuthoring()` + `StartSimulations()` so a manager can replay `NotifyNodeAdded` *between* the two halves — sealing first stops a lifecycle handler authoring nodes nothing would register, starting the simulations last stops a simulated value change preceding its own node''s `OnNodeAdded` — and added `FluentNodeManagerBase.SealConfiguration(builder)` as the one call the generator emits. Both intents are kept. That split is precisely why the asynchronous completion could not simply live inside `SealAsync`: a manager reaching the seam through the split path would silently skip it, which is the failure mode this PR exists to remove. So the asynchronous half became its own step: - `NodeManagerBuilder.CompleteSealAsync(ct)` drains the staged root-notifier registrations, then starts the simulations. `SealAsync(ct)` is `SealGraphAuthoring()` followed by it. - #4432''s synchronous `StartSimulations()` is gone, folded into `CompleteSealAsync`. It had exactly one production caller after the merge, and leaving it standing would have kept a second, synchronous way to activate a builder that skips the staged registrations — the very hazard this PR removes. Activation is now one step with one entry point; the *split* itself stays, because the replay has to sit between the halves. - `SealConfiguration` becomes `SealConfigurationAsync(builder, ct)`: seal the graph, replay `NotifyNodeAdded`, then `CompleteSealAsync`. Activation still happens last, so #4432''s ordering guarantee holds unchanged. ## Consumers unblocked - **#4434 ("Reimplement the GDS ApplicationsNodeManager as a generated fluent node manager")** adds `OnAddressSpaceReadyAsync` to `FluentNodeManagerBase` purely because, in its own words, *"`Configure` is a `partial void` and cannot await"*. That hook is unnecessary once `ConfigureAsync` exists — the GDS manager overrides `ConfigureAsync` instead, and the ordering it wants (predefined nodes → async setup → `Configure`) is what the generator now emits. #4434 has not merged yet, so it can drop its addition rather than have it simplified afterwards. - **Branch `romanett/fluent-node-behaviors`** moved `SimulationRegistry`, `EventSourceRegistry`, `MonitoredSourceRegistry` and the alarm wiring onto `AttachToType`/`Attach` for *release* only, because activation ran from `CompleteConfigureAsync` and most managers only called the synchronous `Seal()`. Every seal site now reaches `SealAsync`, so registry and alarm *setup* can move onto behavior activation too — the remaining half of that work. ## Tests - New `DiNodeManagerConfigureAsyncTests` drives the real `DiNodeManager.CreateAddressSpaceAsync` and asserts the hook runs once with an attached builder, that nodes it stages are registered, that their references to externally owned nodes are published, and that the builder is sealed before `CreateAddressSpaceAsync` returns. - `PublishTests` now pins the root-notifier deferral contract: registration is staged during `Configure`, lands only on the seal, and draining twice does not double-register. - Every `Seal()` call site in the test suites became `await ...SealAsync()`, including the three #4432 added in `NodeSetImportBuilderTests` and `SimulationBuilderExtensionsTests`. ## Related Issues No tracking issue was opened for this; it is the shared prerequisite extracted from #4434 and branch `romanett/fluent-node-behaviors`, both of which had independently worked around the same synchrony. Happy to open one if maintainers want the ADR trail. ## Checklist - [x] 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. - [ ] 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. ### What was actually run locally `UA.slnx` builds clean. Post-merge on **net10.0**: `Opc.Ua.Server.Tests` 5020/0 and `Opc.Ua.SourceGeneration.Core.Tests` 3813/0. Pre-merge the same run plus `Opc.Ua.Di.Tests` 425/0, `Opc.Ua.Robotics.Tests` 627/0, `Opc.Ua.ISA95.Tests` 136/0, `Opc.Ua.Positioning.Tests` and `Opc.Ua.Robotics.Intent.Tests` were all green; `MinimalRobotServer` was also started directly to confirm the hosted robotics path comes up. Three caveats, so the unchecked box above is not a mystery: the full suite was **not** run against .NET Framework locally; `Opc.Ua.OpenUsd.Tests` has one pre-existing failure on this machine (`StructuredCartesianCoordinatesAreAccepted` expects `"1.5"` and gets `"1,5"` — a German-locale decimal separator, unrelated to this change); and one pre-existing `CA1861` warning comes from `NodeSetImportIntegrationTests.cs`, which is byte-identical to `master` on this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
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 |