Repository navigation
CAMEL-25690: camel-milo - answer a producer node with an unknown namespace URI instead of reconnecting - #28045
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Well-structured fix for CAMEL-25690. The root cause was correctly identified: lookupNamespace() could pass a null index to toNodeId(), which caused an NPE completing the future exceptionally and triggering the reconnect logic.
All changes are correct:
lookupNamespace()now returnsnullfor unknown namespace URIs instead of NPE-triggering the futurewrite(),call(),readValues(), andbrowse()all intercept null NodeId and return the appropriate OPC UA status codes locally — no server round-trip neededreadValues()iterator alignment is correct:knownis built in the same order asnodeIds, so the per-element reconstruction is safeknown.isEmpty()guards correctly avoid empty server requests (which would returnBad_NothingToDoper spec)putSubscriptions()refactor is functionally equivalent — oldlookupMonitoredNode()did the same synchronous null-guarded lookup- Tests cover all four fixed code paths with mixed and unknown-only scenarios, and verify that the connection is preserved
- Upgrade guide accurately describes behavior before and after
✅ APPROVE
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…space URI instead of reconnecting The producers resolved the namespace URI (nsu=) of a node ID through the namespace table of the server and failed with a NullPointerException when the URI was not in it. The write, method call, read or browse took that for a lost connection: the client reconnected, which also recreated the subscriptions of the consumers that share the connection. The consumer and the producers now share one node lookup, which completes with null for such a URI, as the lookup of the consumer has done since CAMEL-25671. A producer answers such a node like a node the server does not have, without sending it: Bad_NodeIdUnknown for a write and for that node of a read or browse, and Bad_NodeIdUnknown or Bad_MethodInvalid for a method call. The other nodes of the request, the connection and its consumers are not affected. The method call answer does not reach the exchange yet, because SubscriptionManager.call drops every call result (CAMEL-25694). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
0fbe4c9 to
ec1e6a7
Compare
|
Rebased onto main after #27910 was merged. The conflicts were only textual. The Claude Code on behalf of etzel-mes |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @etzel-mes. I checked each producer path against main. There, toNodeId throws a NullPointerException for the missing index, and the handleAsync guard turns it into handleConnectionFailure and returns null. So an awaited write or read completed with a null body, and the browse producer failed with an NPE on browseResults.values(). The upgrade-guide note describes this correctly.
With the change, the lookup completes with null, and the write, call, read and browse answer such a node without asking the server. A mixed read keeps the requested order, because the known values are taken in request order. A read or browse of only unknown nodes sends no request. The local answers go through the same guard as real server answers, so they will get whatever handling #27956 adds. The consumer behaves as before: lookupNamespace(...).get() does the same as the old lookupMonitoredNode.
The component doc and its catalog copy are identical. The new test follows the JUnit style of the module and has no sleeps. The current head is a rebase of the commit gnodet-bot reviewed (the range-diff shows only context changes), and it merges cleanly with #28049 and #27956.
One nit inline. Approving.
Claude Code on behalf of Claus Ibsen
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 8 of 706 tested, 25 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (3 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (25)
All tested modules (35 modules, 4m 46s total)Total reactor time: 4m 46s
Top 20 slowest modules:
|
Description
CAMEL-25690.
The
milo-clientproducer (write, method call, read) and themilo-browseproducer resolve the namespace URI (nsu=) of a node ID through the namespace table of the server. When the URI was not in the table, the lookup failed with aNullPointerException, and the producer took it for a lost connection: the client reconnected, which also recreated the subscriptions of the consumers that share the connection, and a read lost the values of its other nodes.Why this matters in automation: a namespace URI is configuration, and it goes stale when a server is reconfigured or replaced. A route with a stale URI then reconnected the client on every request. I checked this against a server that is not built on Milo (Microsoft opc-plc 2.15.4, on the OPC Foundation .NET stack). Without this change, one awaited write to such a node replaced the
OpcUaClientof the connection, and the requests sent right after it failed withNo connected: a write and two method calls that the server would have answered itself. One such browse reconnected the browse connection.The change:
lookupNamespaceresolves the namespace URI or index, completes withnullfor a URI that the server does not know, and replaces the consumer'slookupMonitoredNode. The consumer reports such a node withBad_NodeIdUnknownand resolves it under the lock, both as before; its lookup still blocks while the monitored items are created, as before.Bad_NodeIdUnknown; a read returnsBad_NodeIdUnknownfor that node in its position, and a browse under that node ID, both with the results of the other nodes; a method call returnsBad_NodeIdUnknownfor an unknown object andBad_MethodInvalidfor an unknown method. A request without such a node takes the same path as before. A read or browse of only such nodes sends no request, because a server answers an empty one withBad_NothingToDo, which the producer takes for a lost connection. A DEBUG line names the node, because a write that does not wait for its result reports nothing else.ProducerUnknownNamespaceTest: a write, and reads and browses of such a node next to a known node and alone, get these results, and a method call with an unknown method or object completes without an error. Each connection keeps itsOpcUaClient, and after the write, the reads and the calls a consumer on the same connection receives the next value. Without the fix all four tests fail, and the client reconnects in each.Against opc-plc with the change, each answer matched the answer of the server for a node it does not have (
ns=99;s=…, or an unknown identifier in a known namespace): one read of six nodes returnedGood, Good, Bad_NodeIdUnknown, Good, Bad_NodeIdUnknown, Bad_NodeIdUnknownin the requested order, and a browse returned the references ofObjectsnext toBad_NodeIdUnknown. Both connections kept theirOpcUaClient, and a consumer on the client connection kept receiving its counter. On top of #27956, the two method calls also returned what the server returns for an unknown method of an object (Bad_MethodInvalid) and for an unknown object (Bad_NodeIdUnknown).This PR does not change the four request guards in
SubscriptionManagerthat #27956 (CAMEL-25694, @gnodet) changes, and the two merge without conflicts in either order. The call guard on main drops every method call result, so for now a method call with such a node only keeps the connection, and its answer reaches the exchange with #27956. The docs and the test therefore leave the call answer out; I can add it to both once #27956 is merged. On top of #27956, such call assertions and the tests of #27956 already pass.The unchecked lookup dates back to the first client implementation and came back with the component in 84b7e90; this change does not revert any later work.
Checked with
-Psourcecheck clean verifyon the module (83 tests), four more runs of the new and the affected test classes with-Dsurefire.rerunFailingTestsCount=0, one of them with-XX:ActiveProcessorCount=1, and the new test together with the tests of #27956 on top of it.@davsclaus @oscerd could you take a look?
Target
mainbranch)Tracking
Apache Camel coding standards and style
I checked that each commit in the pull request has a meaningful subject line and body.
I have run
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Claude Code on behalf of etzel-mes