fix(galaxy): authoritative handle resolution + review cleanups
Make SubscriptionRegistry.TryResolveItemHandle confirm a live subscription genuinely binds fullRef->handle (via the reverse index) rather than trusting the forward-map hint + a bare liveness check. Fixes the cross-ref-same-handle hazard (wrong-tag borrow) while preserving the legitimate multiple-subscriptions-per-tag borrow. Adds cross-ref + same-ref-multi-sub tests; drops a duplicate SubscriptionEntry <summary>; documents the writer's supervisory-advise reconnect lifecycle.
This commit is contained in:
@@ -210,7 +210,9 @@ public sealed class GatewayGalaxyDataWriter : IGalaxyDataWriter
|
|||||||
/// a regular Advise instead; we don't log in, so we use the supervisory context.)
|
/// a regular Advise instead; we don't log in, so we use the supervisory context.)
|
||||||
/// The gateway client doesn't expose a typed method, so we build the <see cref="MxCommand"/>
|
/// The gateway client doesn't expose a typed method, so we build the <see cref="MxCommand"/>
|
||||||
/// and route through <c>InvokeAsync</c> (same pattern as <see cref="InvokeWriteSecuredAsync"/>).
|
/// and route through <c>InvokeAsync</c> (same pattern as <see cref="InvokeWriteSecuredAsync"/>).
|
||||||
/// Idempotent per handle for the session lifetime.
|
/// Idempotent per handle for the session lifetime. The keyed-by-handle dedupe is reset on a
|
||||||
|
/// session reconnect via <see cref="InvalidateHandleCaches"/>, so a fresh session always
|
||||||
|
/// re-advises — the supervisory state never outlives the handle it was taken against.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private async Task EnsureSupervisoryAdvisedAsync(
|
private async Task EnsureSupervisoryAdvisedAsync(
|
||||||
MxGatewaySession session, int serverHandle, int itemHandle, CancellationToken ct)
|
MxGatewaySession session, int serverHandle, int itemHandle, CancellationToken ct)
|
||||||
|
|||||||
@@ -124,18 +124,31 @@ internal sealed class SubscriptionRegistry
|
|||||||
/// <summary>
|
/// <summary>
|
||||||
/// Resolve the live MXAccess item handle a current subscription holds for <paramref name="fullRef"/>,
|
/// Resolve the live MXAccess item handle a current subscription holds for <paramref name="fullRef"/>,
|
||||||
/// or null when no live subscription covers it. The Galaxy writer borrows this handle to skip a
|
/// or null when no live subscription covers it. The Galaxy writer borrows this handle to skip a
|
||||||
/// redundant AddItem round-trip on the first write to an already-subscribed tag. Guarded by the
|
/// redundant AddItem round-trip on the first write to an already-subscribed tag.
|
||||||
/// authoritative live-handle set (<c>_subscribersByItemHandle</c>) so a stale forward-map entry
|
|
||||||
/// can never hand out a dead handle.
|
|
||||||
/// </summary>
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// The <c>_itemHandleByFullRef</c> forward map is only a hint — resolution is authoritative:
|
||||||
|
/// it confirms a <em>live</em> subscription actually binds this <paramref name="fullRef"/> to
|
||||||
|
/// the candidate handle (via the per-entry reverse index), so a stale forward entry can never
|
||||||
|
/// hand the writer a dead handle, nor a handle that now belongs to a different tag. This keeps
|
||||||
|
/// a tag resolvable while ANY subscription still binds it (the legitimate
|
||||||
|
/// multiple-driver-subscriptions-per-tag case) yet returns null the moment the last one drops.
|
||||||
|
/// </remarks>
|
||||||
/// <param name="fullRef">The dotted tag full reference (e.g. <c>TestMachine_002.TestFloat</c>).</param>
|
/// <param name="fullRef">The dotted tag full reference (e.g. <c>TestMachine_002.TestFloat</c>).</param>
|
||||||
/// <returns>The live item handle, or null when none is currently subscribed.</returns>
|
/// <returns>The live item handle, or null when none is currently subscribed.</returns>
|
||||||
public int? TryResolveItemHandle(string fullRef)
|
public int? TryResolveItemHandle(string fullRef)
|
||||||
{
|
{
|
||||||
if (fullRef is null) return null;
|
if (fullRef is null) return null;
|
||||||
if (_itemHandleByFullRef.TryGetValue(fullRef, out var handle)
|
if (!_itemHandleByFullRef.TryGetValue(fullRef, out var handle)) return null;
|
||||||
&& _subscribersByItemHandle.ContainsKey(handle))
|
if (!_subscribersByItemHandle.TryGetValue(handle, out var subs)) return null; // dead handle
|
||||||
return handle;
|
|
||||||
|
// Confirm a live subscription genuinely binds fullRef -> handle (not a stale forward entry
|
||||||
|
// pointing at a handle the gateway has since associated with another tag).
|
||||||
|
foreach (var subId in subs)
|
||||||
|
if (_bySubscriptionId.TryGetValue(subId, out var entry)
|
||||||
|
&& entry.FullRefByItemHandle.TryGetValue(handle, out var liveRef)
|
||||||
|
&& string.Equals(liveRef, fullRef, StringComparison.OrdinalIgnoreCase))
|
||||||
|
return handle;
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -201,7 +214,6 @@ internal sealed class SubscriptionRegistry
|
|||||||
/// (Driver.Galaxy-012). Failed bindings (item handle ≤ 0) are excluded from the
|
/// (Driver.Galaxy-012). Failed bindings (item handle ≤ 0) are excluded from the
|
||||||
/// index because the EventPump only dispatches for positive handles.
|
/// index because the EventPump only dispatches for positive handles.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <summary>Per-subscription bookkeeping entry.</summary>
|
|
||||||
private sealed class SubscriptionEntry
|
private sealed class SubscriptionEntry
|
||||||
{
|
{
|
||||||
/// <summary>Gets the subscription identifier.</summary>
|
/// <summary>Gets the subscription identifier.</summary>
|
||||||
|
|||||||
+39
@@ -114,4 +114,43 @@ public sealed class SubscriptionRegistryHandleResolveTests
|
|||||||
// must return null regardless of whether the forward entry was cleaned up.
|
// must return null regardless of whether the forward entry was cleaned up.
|
||||||
registry.TryResolveItemHandle("Tag.A").ShouldBeNull();
|
registry.TryResolveItemHandle("Tag.A").ShouldBeNull();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// A tag may legitimately appear in multiple driver subscriptions (separate OPC UA monitored
|
||||||
|
/// items on the same Galaxy attribute) — they share one gw item handle. Removing ONE of them
|
||||||
|
/// must keep the tag resolvable while another subscription still binds it, so the writer keeps
|
||||||
|
/// borrowing instead of falling back to AddItem.
|
||||||
|
/// </summary>
|
||||||
|
[Fact]
|
||||||
|
public void Remove_OneOfTwoSubscribersForSameRef_StillResolves()
|
||||||
|
{
|
||||||
|
var registry = new SubscriptionRegistry();
|
||||||
|
registry.Register(1, [new TagBinding("Tag.A", 5)]);
|
||||||
|
registry.Register(2, [new TagBinding("Tag.A", 5)]);
|
||||||
|
|
||||||
|
registry.Remove(1);
|
||||||
|
|
||||||
|
// sub2 still binds Tag.A -> 5, so the borrow must still be offered.
|
||||||
|
registry.TryResolveItemHandle("Tag.A").ShouldBe(5);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Authoritative resolution: if two DIFFERENT refs ever map to the same numeric handle and the
|
||||||
|
/// subscription for one is removed, that ref must NOT resolve to the handle that now belongs to
|
||||||
|
/// the other ref — a wrong-tag write would be the worst outcome. Resolution confirms a live
|
||||||
|
/// subscription genuinely binds fullRef -> handle, not just that the handle is alive.
|
||||||
|
/// </summary>
|
||||||
|
[Fact]
|
||||||
|
public void Remove_CrossRefSameHandle_DoesNotResolveToTheOtherRefsHandle()
|
||||||
|
{
|
||||||
|
var registry = new SubscriptionRegistry();
|
||||||
|
registry.Register(1, [new TagBinding("Tag.A", 5)]);
|
||||||
|
registry.Register(2, [new TagBinding("Tag.B", 5)]);
|
||||||
|
|
||||||
|
registry.Remove(1);
|
||||||
|
|
||||||
|
// Tag.A's subscription is gone; handle 5 is still alive but now only Tag.B binds it.
|
||||||
|
registry.TryResolveItemHandle("Tag.A").ShouldBeNull();
|
||||||
|
registry.TryResolveItemHandle("Tag.B").ShouldBe(5);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user