mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-21 00:56:28 +08:00
636912bd4c
# Description Two things, both surfaced by the live NodeManager registration work in #4094. > **Rebased on #4114.** Upstream landed #4114 (`cc4667d0b`) while this PR was open; it fixes the > same #4113 root cause. `origin/master` is merged in and the conflicts were resolved **in favour of > the upstream API shape** — `CompleteAsync(IMessageProcessor, uint subscriptionId, ct)` with the > last known server id passed as an argument. The `IMessageProcessor.LastServerId` member this > branch had added is dropped again. What remains here is the orthogonal hardening #4114 does not > cover, plus all of the #4100 work. ## 1. Remaining subscription-retirement hardening (on top of #4114) #4114 fixed the id-collision itself. Three related holes in the same paths are still open: - **Retirement was not atomic with removal.** `RetireSubscriptionId` is now called *under* `m_subscriptionLock` from both `CompleteAsync` and `RemoveSecondaryPartitionAsync`. `GetById` resolves incoming publish responses under the same lock, so a worker could otherwise observe the subscription as removed while its id had not yet reached the retired-id ring — and send a redundant `DeleteSubscriptions` for an already-retired id. `RemoveSecondaryPartitionAsync` did not record the retired id at all. - **The retired-id ring was far too small.** `kMaxSubscriptionHistory` 10 → 256, so subscription churn cannot evict live history. - **The orphan delete was unguarded.** The publish worker no longer sends `DeleteSubscriptions` for an unresolved id while any subscription is still awaiting its id. A publish response can overtake the `CreateSubscription` continuation that assigns the id, and deleting in that window kills a healthy subscription. Leaving a genuine orphan alive until its lifetime expires is the safe trade-off. - **`PublishControllerAsync` lost resize signals.** It re-created its `AsyncAutoResetEvent` waiter on every loop iteration; the abandoned waiter stayed enqueued and swallowed a later `Set()`, so the publish worker pool stopped resizing. This is a second, latent starvation path independent of the id collision. The wait is now created once and only re-armed after it completes. ## 2. Managed client does not refresh the namespace table on model change (#4100) `NodeManagerLifecycle.NotifyCommittedChangeAsync` updates `NamespaceArray`, bumps `UrisVersion`, and then reports a **`BaseModelChangeEvent`** — an event shape that carries no `Changes` payload. The client tracker *did* observe it and cleared the `INodeCache`, but it held only an `IStreamingSubscription`, an `INodeCache` and an `ILogger`, so nothing could re-read the namespace table. A tracking client therefore resolved NodeIds from a namespace added by a live NodeManager registration against a stale `NamespaceUris` table. - **New `INamespaceTableRefresher`** (`NamespaceUris` + `FetchNamespaceTablesAsync`). The member signatures match what `Session` and `ManagedSession` already expose, so both satisfy the interface without any new members. `ISession` is deliberately left untouched — moving its members onto a base interface would be a binary break for 1.5.378 consumers. - **`ModelChangeTracker`** takes the refresher as an optional *trailing* constructor argument, which keeps every existing call site source-compatible and makes `null` the opt-out. A refresh runs when the event carries no per-node detail, when an affected node is `Server_NamespaceArray`, or when an affected namespace index is beyond the client's table. It runs **before** the cache is invalidated and before `ModelChanged` is raised, so subscribers that re-browse from the handler already see the new uris. A failing refresh is logged and swallowed. - **`ModelChangedEventArgs.NamespaceTableRefreshed`** reports the refresh to applications. - **`ManagedSession.EnableModelChangeTrackingAsync`** wires the session in, so opting into tracking is all an application has to do — no new option, no change to the positional `CreateAsync` signature. - **`ModelChange.TypeDefinition`** changes from `NodeId?` to `NodeId`. `NodeId` is a readonly struct implementing `INullable` and must not be wrapped in `System.Nullable<T>`; use `NodeId.Null` and `IsNull` instead. The type is 2.0-preview only (absent from 1.5.378), and the only producer is the tracker itself, which already assigns a non-null `AffectedType`. Per review feedback the type is also now a `readonly record struct`, matching `DataValueChange` / `EventNotification`. ## Tests Every new regression test was run against a temporarily re-introduced defect and **fails** there: | Test | Covers | |---|---| | `CompleteAsyncRetiresOnlyTheCompletedSubscriptionWhenOthersArePendingCreationAsync` | retire-by-identity (also guards #4114's fix) | | `CompleteAsyncRemembersRetiredServerIdAfterIdWasResetAsync` | retired-id history | | `PublishWorkerDefersDeletingUnknownSubscriptionWhileCreationIsPendingAsync` | orphan-delete guard | | `PublishControllerKeepsResizingAfterWorkersExitAsync` | publish-controller signal loss | | `ConcurrentAddAndCompleteRetireOnlyCompletedSubscriptionsAsync` | parallel add/retire registry integrity | | `ConcurrentSubscriptionChurnNeverStarvesASubscription` (integration) | #4113 repro against the reference server | | 7 × `ModelChangeTrackerTests` namespace-refresh cases | each trigger, no-trigger, throwing refresher, `null` refresher, ordering | | `LiveNodeManagerAddRefreshesTheClientNamespaceTable` (integration) | #4100 repro — times out without the wiring | `FakeMessageAckQueue` records both the completed id (upstream's assertions) and the completing instance (the identity assertion in `MessageProcessorTests`), so neither side's coverage was lost in the merge. Results after merging `origin/master`: - `Opc.Ua.Client.Tests` — **2119/2119** pass on net10.0; the new `ModelChange` and `SubscriptionManager` fixtures also pass on **net48**. - `Opc.Ua.Subscriptions.Tests` churn + coverage fixtures — **28/28** (net10.0); churn fixture also passes on net48. - `Opc.Ua.Server.Tests` `NodeManagerLifecycle*` — **49/49** on net10.0; the new fixture also passes on net48. - No new analyzer warnings; only the three pre-existing `CA1873` hits in `NodeCacheResolver.cs` and `ClassicSubscriptionEngine.cs` (untouched files) remain. ## Documentation - `docs/ModelChangeTracking.md` — new **Namespace table refresh** section with the trigger table, updated `ModelChangedEventArgs` / `ModelChange` snippets, updated manual-construction snippet, and a quick-reference row. - `docs/NodeManagerRegistration.md` — client-side half of the **Namespaces** section. No `docs/migrationguide.md` entry: every changed type is either `internal` or 2.0-preview-only. ## Related Issues - Fixes #4100 - Follow-up hardening for #4113 (closed by #4114) - Related to #4094, #3993 ## 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. - [x] 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. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e22f9b3c-1682-40bd-8b72-31a11352fa2c
184 lines
7.6 KiB
C#
184 lines
7.6 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/
|
|
* ======================================================================*/
|
|
|
|
// CA2016: integration tests intentionally call cleanup in finally without forwarding the test
|
|
// cancellation token. The test CT may already be cancelled (the [CancelAfter] timeout), which
|
|
// would prevent cleanup from running. CloseAsync/DisposeAsync must complete regardless.
|
|
#pragma warning disable CA2016
|
|
|
|
using System;
|
|
using System.Collections.Concurrent;
|
|
using System.Linq;
|
|
using System.Threading;
|
|
using System.Threading.Tasks;
|
|
using NUnit.Framework;
|
|
using Opc.Ua.Client;
|
|
using Opc.Ua.Client.Subscriptions;
|
|
using Opc.Ua.Client.TestFramework;
|
|
using ManagedSessionType = Opc.Ua.Client.ManagedSession;
|
|
|
|
namespace Opc.Ua.Subscriptions.Tests
|
|
{
|
|
/// <summary>
|
|
/// Regression coverage for
|
|
/// <see href="https://github.com/OPCFoundation/UA-.NETStandard/issues/4113"/>:
|
|
/// adding and disposing subscriptions concurrently on a single
|
|
/// <see cref="ManagedSessionType"/> silently starved a fraction of them.
|
|
/// A subscription resets its server side id before its message processor
|
|
/// completes, so retiring it by id evicted an arbitrary other subscription
|
|
/// that had been added but not created yet. The evicted subscription was
|
|
/// then missing from the publish dispatch registry and never received a
|
|
/// notification.
|
|
/// </summary>
|
|
[TestFixture]
|
|
[Category("Client")]
|
|
[Category("ManagedSession")]
|
|
[Category("SubscriptionManager")]
|
|
[SetCulture("en-us")]
|
|
[SetUICulture("en-us")]
|
|
public class ConcurrentSubscriptionChurnTests : ClientTestFramework
|
|
{
|
|
[OneTimeSetUp]
|
|
public override Task OneTimeSetUpAsync()
|
|
{
|
|
SupportsExternalServerUrl = true;
|
|
SingleSession = false;
|
|
return OneTimeSetUpCoreAsync(securityNone: true);
|
|
}
|
|
|
|
[OneTimeTearDown]
|
|
public override Task OneTimeTearDownAsync()
|
|
{
|
|
return base.OneTimeTearDownAsync();
|
|
}
|
|
|
|
[SetUp]
|
|
public override Task SetUpAsync()
|
|
{
|
|
return base.SetUpAsync();
|
|
}
|
|
|
|
[TearDown]
|
|
public override Task TearDownAsync()
|
|
{
|
|
return base.TearDownAsync();
|
|
}
|
|
|
|
[Test]
|
|
[CancelAfter(180_000)]
|
|
public async Task ConcurrentSubscriptionChurnNeverStarvesASubscription(
|
|
CancellationToken ct)
|
|
{
|
|
const int kConcurrency = 6;
|
|
const int kIterationsPerWorker = 8;
|
|
|
|
ConfiguredEndpoint endpoint = await ClientFixture
|
|
.GetEndpointAsync(ServerUrl, SecurityPolicies.None)
|
|
.ConfigureAwait(false);
|
|
|
|
ManagedSessionType session = await new ManagedSessionBuilder(
|
|
ClientFixture.Config, Telemetry)
|
|
.UseEndpoint(endpoint)
|
|
.WithSessionName(nameof(ConcurrentSubscriptionChurnNeverStarvesASubscription))
|
|
.WithSessionTimeout(TimeSpan.FromSeconds(120))
|
|
.ConnectAsync(ct)
|
|
.ConfigureAwait(false);
|
|
|
|
var starved = new ConcurrentBag<int>();
|
|
try
|
|
{
|
|
Assert.That(
|
|
session.TryGetSubscriptionManager(out ISubscriptionManager? manager),
|
|
Is.True);
|
|
Assert.That(manager, Is.Not.Null);
|
|
|
|
await Task.WhenAll(Enumerable.Range(0, kConcurrency)
|
|
.Select(worker => Task.Run(async () =>
|
|
{
|
|
for (int i = 0; i < kIterationsPerWorker; i++)
|
|
{
|
|
var handler = new RecordingSubscriptionHandler();
|
|
ISubscription subscription = session.AddSubscription(
|
|
handler,
|
|
new Client.Subscriptions.SubscriptionOptions
|
|
{
|
|
PublishingInterval = TimeSpan.FromMilliseconds(200),
|
|
PublishingEnabled = true,
|
|
KeepAliveCount = 10,
|
|
LifetimeCount = 1000
|
|
});
|
|
try
|
|
{
|
|
Assert.That(
|
|
subscription.TryAddMonitoredItem(
|
|
"CurrentTime",
|
|
VariableIds.Server_ServerStatus_CurrentTime,
|
|
o => o with
|
|
{
|
|
SamplingInterval =
|
|
TimeSpan.FromMilliseconds(100),
|
|
QueueSize = 1
|
|
},
|
|
out _),
|
|
Is.True);
|
|
|
|
bool gotData = await handler
|
|
.WaitForFirstDataAsync(
|
|
TimeSpan.FromSeconds(20), ct)
|
|
.ConfigureAwait(false);
|
|
if (!gotData)
|
|
{
|
|
starved.Add((worker * kIterationsPerWorker) + i);
|
|
}
|
|
}
|
|
finally
|
|
{
|
|
await subscription.DisposeAsync().ConfigureAwait(false);
|
|
}
|
|
}
|
|
}))).ConfigureAwait(false);
|
|
|
|
Assert.That(starved, Is.Empty,
|
|
$"{starved.Count} of {kConcurrency * kIterationsPerWorker} " +
|
|
"subscriptions never received a data change notification.");
|
|
|
|
// Every subscription was disposed, so the manager registry
|
|
// must be empty again — an evicted subscription would leave
|
|
// the counts inconsistent.
|
|
Assert.That(manager!.Count, Is.Zero);
|
|
}
|
|
finally
|
|
{
|
|
await session.CloseAsync().ConfigureAwait(false);
|
|
await session.DisposeAsync().ConfigureAwait(false);
|
|
}
|
|
}
|
|
}
|
|
}
|