Node manager refresh: Monitored item retirement support - #4173
Node manager refresh: Monitored item retirement support#4173marcschier wants to merge 4 commits into
Conversation
Restore the bespoke monitored item and subscription retirement mechanism that was dropped when the original WoT binding draft was split into stacked PRs. Port the behavior onto the rewritten NodeManager lifecycle code instead of reverting those files, so immediate reloads retire old-generation monitored items with BadNodeIdUnknown while still disposing the retired NodeManager generation. Co-authored-by: Copilot <[email protected]> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
marcschier
changed the title
| { | ||
| get | ||
| { | ||
| if (m_retirementNotificationPending) |
There was a problem hiding this comment.
wouldnt this leak Monitored items?
(although in a half functional state?
also the RetirementError property is imo not needed and Retire could then be simplified without a parameter.
also i dont like that this adds another check that is done in many places (error prone) and not backed by any opc ua spec (so only skd authors will eventually know about it).
No one can write to that item, so better to just queue the value and remove that checks?
There was a problem hiding this comment.
Thanks - you were right, and on all four points I ended up removing the mechanism rather than defending it. Per point:
3. The check duplicated in many places (your strongest point - fixing it dissolved the others). The stack already has this exact null-object pattern: Detach parks a monitored item on the long-lived CoreNodeManager and every service path already handles that through its IsDetached branch. Retirement was reinventing it the hard way - Retire() nulled NodeManager/ManagerHandle via DetachOwner(), which is the only reason MasterNodeManager and Subscription needed the extra guard (GetRetirementError at ModifyMonitoredItems, DeleteMonitoredItems and SetMonitoringMode, plus IsRetired twice in Subscription). I deleted GetRetirementError and every one of those call sites. Retire() now parks the item on CoreNodeManager exactly like Detach, so retired items flow through the existing IsDetached routing and produce the right result with no retirement-specific check anywhere - a service added to MasterNodeManager later is covered without anyone knowing "retirement" exists.
It is also spec-backed now, which answers "only SDK authors will know": a retired item is indistinguishable from a monitored item whose Node was deleted. OPC UA Part 4 (OPC 10000-4) v1.05.07 section 5.8.4.1 (DeleteNodes) requires the server to send a Bad_NodeIdUnknown notification to monitoring clients when a monitored Node is deleted, with later service calls returning the same status. So the client sees standard behaviour, not a private convention.
2. RetirementError not needed, Retire could drop the parameter. Both gone. IRetirableMonitoredItem is now just void Retire(). The error was never variable - the single caller always passed new ServiceResult(StatusCodes.BadNodeIdUnknown) - so I dropped the parameter, the RetirementError property and the separate DetachOwner() member. In NodeManagerLifecycle the ServiceResult? immediateRetirementError plumbing becomes a plain bool immediateRetirement.
4. No one can write to it, so just queue the value and remove the checks. That is exactly the mechanism now. Retire() disposes the datachange queue handler, sets the terminal Bad_NodeIdUnknown as m_lastValue and notifies the subscription; the value is queued and no writer can reach the item because its owner is the tombstone CoreNodeManager. The IsReadyToPublish short-circuit at line 437 that you anchored on is deleted - publishing goes through the normal ready path.
1. Would this leak monitored items? No - bounded, and now provably so. A retired item stays in the subscription with its terminal value only until the client calls DeleteMonitoredItems (returns Good) or the subscription/session is torn down: the same lifetime as any monitored item on a deleted Node. It is bounded by the items the client created; the server never grows it. The contract test NodeManagerLifecycleTests.ImmediateReloadAsyncReportsBadNodeIdUnknownAndDisposesPriorGenerationAsync walks the whole path - publish returns BadNodeIdUnknown, Modify returns BadNodeIdUnknown, SetMonitoringMode returns Good, and after DeleteMonitoredItems it asserts subscription.MonitoredItemCount is zero. Because Retire() is idempotent (it returns early once parked), the lifecycle's old two-phase retire-then-detach cleanup collapsed into a single call.
Net: 153 insertions, 402 deletions. Opc.Ua.Server.Tests is green (4040 passed, 0 failed) and the multi-TFM Release build is 0 warnings / 0 errors. I have left the thread open - whether this settles it is your call.
Addressing romanett's review feedback on PR #4173, this removes the bespoke "retirement" mechanism for monitored items owned by a NodeManager generation that is retired during an immediate shadow reload, and instead reuses the existing detach-to-CoreNodeManager machinery. Previously a retired item nulled out its owner via DetachOwner(), which forced MasterNodeManager and Subscription to special-case it: an IsRetired / GetRetirementError predicate was re-tested at roughly eight service call sites, and any future ownership-sensitive service had to remember to add the same check or risk a null reference on NodeManager. That is error prone and is not backed by the OPC UA specification, so only SDK authors would ever know about it. Retirement is not a distinct concept: a retired item is simply a detached and deleted item. Retire() now parks the item on the long lived CoreNodeManager (the same null-object owner already used by Detach), marks it deleted, and queues a terminal Bad_NodeIdUnknown, exactly like a monitored Node that was deleted (OPC UA Part 4 section 5.8.4.1). Ownership-sensitive services (modify, delete, set monitoring mode, transfer) then route to the parked owner and are handled by the pre-existing IsDetached branches, so no retirement-specific checks remain. The call is idempotent, which lets the lifecycle collapse its former two-phase retire-then-detach cleanup into a single call. This drops the IsRetired and RetirementError members and the Retire(error) parameter from IRetirableMonitoredItem, the GetRetirementError helper and its three call sites in MasterNodeManager, the IsRetired helper and DetachRetiredMonitoredItems from Subscription, and the ImmediateRetirementError plumbing in NodeManagerLifecycle (now a bool). The client-observable behaviour is unchanged and remains covered by the NodeManagerLifecycleTests contract test. Co-authored-by: Copilot <[email protected]> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
…marcschier/wot-17-retirement
…marcschier/wot-17-retirement
| /// <summary> | ||
| /// Invalidates monitored items owned by a NodeManager before an immediate generation retirement. | ||
| /// </summary> | ||
| internal interface INodeManagerMonitoredItemRetirementTracker |
There was a problem hiding this comment.
this needs to be renamed to indicate a subscription implements this functionality
| ParkOnDetachedOwner(GetDetachedOwner(m_server)); | ||
| m_isDeleted = true; | ||
|
|
||
| if ((MonitoredItemType & MonitoredItemTypeMask.DataChange) != 0) |
There was a problem hiding this comment.
this "semi dispose" is not needed and should be removed imo, just queue the value and thats it.
Also trigger for ItemReadyToPublish is not needed, as the value reports true as soon as the BadNodeID unknown is queued
| return m_monitoredItems.Values | ||
| .Where(monitoredItem => IsOwnedBy(monitoredItem.Value, nodeManager)) | ||
| .All(monitoredItem => | ||
| !monitoredItem.Value.IsDurable && |
There was a problem hiding this comment.
if the subscription is not durable monitored items can also not be durable
| .Where(monitoredItem => IsOwnedBy(monitoredItem, nodeManager)) | ||
| ]; | ||
| if (candidates.Any(monitoredItem => | ||
| monitoredItem.IsDurable || |
There was a problem hiding this comment.
if the subscription is not durable monitored items can also not be durable
2 participants
Summary
Validation
Results: 0 failed / 4048 passed / 8 skipped.