gnodet-bot commented on code in PR #27910:
URL: https://github.com/apache/camel/pull/27910#discussion_r4286084953
##########
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();
Review Comment:
⚠️ **No timeout on `subscription.create()`** — If the OPC UA server becomes
unresponsive after `delete()` has already been called, `create()` will block
indefinitely while holding `lock`. This would stall all routing threads until
the underlying TCP stack eventually times out (often minutes).
Consider applying the same timeout strategy used elsewhere in Milo client
interactions, or documenting that the caller relies on TCP-level timeouts.
##########
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))
{
Review Comment:
⚠️ **Blocking network I/O under lock** — The method holds `lock` across
`subscription.delete()` → `subscription.create()` →
`subscription.createMonitoredItems()`, all of which are synchronous network
roundtrips to the OPC UA server.
This differs from the existing `performAndEvalConnect()` pattern, where
`performConnect()` (including `subscription.create()`) runs **outside** the
lock, and `lock.lock()` is only acquired afterwards to update `this.connected`.
Any Camel routing thread calling `write()`, `read()`, `activate()`,
`deactivate()`, or `dispose()` will block on `lock.lock()` for the full
duration of these network roundtrips.
If this is intentional (e.g. to prevent concurrent recreation attempts),
please add a brief Javadoc note explaining the trade-off.
--
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]
