davsclaus commented on code in PR #27910:
URL: https://github.com/apache/camel/pull/27910#discussion_r4286228686
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -683,6 +697,37 @@ public OpcUaClient getOpcUaClient() {
}
}
+ /**
+ * Deletes and creates the subscription of the current connection again,
inside the existing session, and creates
+ * its monitored items again. Does nothing if the subscription no longer
belongs to the current connection or has
+ * already been recreated since the given id was observed. Falls back to
rebuilding the connection if this fails.
+ */
+ private void recreateSubscription(final OpcUaSubscription subscription,
final Optional<UInteger> id) {
+ lock.lock();
+ try {
+ if (this.disposed || this.connected == null ||
this.connected.manager != subscription
+ || !subscription.getSubscriptionId().equals(id)) {
+ LOG.debug("Subscription {} is stale or already recreated",
id.orElse(null));
+ return;
+ }
+ try {
+ subscription.delete();
+ } catch (final UaException e) {
+ LOG.debug("Failed to delete subscription {}", id.orElse(null),
e);
+ subscription.reset();
+ }
Review Comment:
**`reset()` here can leave a live subscription running on the server.**
When the server no longer has the subscription, `delete()` gets
`Bad_SubscriptionIdInvalid`. Milo then resets the object itself in
`applyDeleteResponse` before it throws, so this `reset()` is a no-op in the
case the PR targets. It only matters for other failures: a request timeout on a
slow server, or a closed connection. In those cases the subscription may still
exist on the server. After `reset()` + `create()`, the old one is abandoned in
the same session, where it keeps its monitored items. It can stay alive as long
as the session keeps sending Publish requests, and Milo discards its responses.
PLC servers often have tight subscription limits, so a leak like this matters
there.
Suggestion: only continue in place when the server confirmed the
subscription is gone. Otherwise let the outer `catch` fall back to
`handleConnectionFailure`, which closes the session and the subscriptions on it:
```suggestion
try {
subscription.delete();
} catch (final UaException e) {
if (e.getStatusCode().getValue() !=
StatusCodes.Bad_SubscriptionIdInvalid) {
// the subscription may still exist on the server,
rebuilding the connection closes its session
throw e;
}
LOG.debug("Subscription {} no longer exists on the server",
id.orElse(null));
}
```
(For the `Bad_Timeout` path, Milo has already reset the subscription, so
`delete()` returns without calling the server.)
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -683,6 +697,37 @@ public OpcUaClient getOpcUaClient() {
}
}
+ /**
+ * Deletes and creates the subscription of the current connection again,
inside the existing session, and creates
+ * its monitored items again. Does nothing if the subscription no longer
belongs to the current connection or has
+ * already been recreated since the given id was observed. Falls back to
rebuilding the connection if this fails.
+ */
+ private void recreateSubscription(final OpcUaSubscription subscription,
final Optional<UInteger> id) {
+ lock.lock();
+ try {
+ if (this.disposed || this.connected == null ||
this.connected.manager != subscription
+ || !subscription.getSubscriptionId().equals(id)) {
+ LOG.debug("Subscription {} is stale or already recreated",
id.orElse(null));
+ return;
+ }
+ try {
+ subscription.delete();
+ } catch (final UaException e) {
+ LOG.debug("Failed to delete subscription {}", id.orElse(null),
e);
+ subscription.reset();
+ }
+ subscription.create();
+ if
(!subscription.createMonitoredItems().stream().allMatch(MonitoredItemServiceOperationResult::isGood))
{
+ throw new UaException(StatusCodes.Bad_UnexpectedError, "Failed
to create the monitored items again");
+ }
Review Comment:
**One item that cannot be created again rebuilds the whole connection.**
`isGood()` needs both the service result and the operation result to be
good, so one operation-level failure fails the whole check. For example, the PR
description mentions a server configuration reload: a node removed by that
reload comes back as `Bad_NodeIdUnknown`. The check then throws, and
`handleConnectionFailure` closes the session for every consumer and producer on
the connection. That is the cost the PR sets out to avoid. It also handles this
case differently from `Connected.putSubscriptions`. There, operation-level
failures are removed from the subscription, reported once to their consumer as
a bad `DataValue` (outside the lock), and do not cause a rebuild. Only a bad
*service* result rebuilds.
Could the recreate reuse that handling, so that only a rejected service call
falls back to the rebuild? Then the failed item should also be removed from
`goodSubscriptions`, so that a later `deactivate` does not try to delete it. A
test that removes one monitored node on the server before the recreate would
cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
