Repository navigation
CAMEL-25912: camel-milo - client producers no longer wait for the manager lock or rebuild the connection - #28371
Conversation
…ager lock or rebuild the connection
Problem: SubscriptionManager used one lock for the subscription work and
the producer requests. Registering and deleting monitored items, recreating
a subscription and the periodic retry of refused nodes hold the lock across
a blocking OPC UA service call. write, call, readValues, browse and
getOpcUaClient() took the same lock, although under it they only read the
current connection and start an asynchronous request. A producer therefore
waited for unrelated subscription work on the same connection: one round
trip normally, about 10 s while the link was dropped silently (until the
keep-alive declared the session lost), up to requestTimeout when the server
answered keep-alives but not the call. Even without the lock, the outcome
of a producer request waited for the refused-node retry: the retry ran its
blocking CreateMonitoredItems call on the MiloClient executor, which also
completes the producer requests and has a single thread on a machine with
one processor. On a real server that held CreateMonitoredItems for 30 s and
70 s with --cpus=1, writes it answered in 10-25 ms completed only after
29.0 s and 59.1 s, when the retry returned.
Cause: the lock guarded the read of Connected.client, which dispose()
nulled. A failed producer request also called handleConnectionFailure,
which tore down whatever connection was current when the failure arrived
(possibly a replacement) and held the lock across client.disconnect().
Fix:
- The producers and getOpcUaClient() read the volatile connected without
the lock. Connected.client is final and no longer nulled by dispose(), so
a request on a connection disposed meanwhile fails with Milo's own
UaException (Bad_SessionClosed after the disconnect, Bad_ConnectionClosed
for one in flight) instead of a NullPointerException. dispose() is
idempotent through listener.closed.
- SubscriptionManager.dispose() nulls connected under the lock, and
handleConnectionFailure() nulls it before it disconnects, so
getOpcUaClient() keeps returning null while there is no usable
connection, as documented. handleConnectionFailure() sets
lastConnectFailure first, so a producer that finds no connection during
the teardown gets that failure as the cause of its not-connected result.
- A failed producer request no longer calls handleConnectionFailure: the
exchange gets the failure (CAMEL-25694) and the connection is kept. A lost
session is recovered by Milo, a session the server no longer has fails the
subscription transfer, which rebuilds the connection, and the watchdog
handles a subscription that stops publishing. The outcome is still
completed on the MiloClient executor with whenCompleteAsync, so the route
never continues on a Milo thread.
- The refused-node retry runs on the MiloClientReconnect thread, which
already ran its timer, instead of handing the attempt to the MiloClient
executor. Trade-off: a stalled attempt keeps that thread busy for up to
requestTimeout, and a reconnect queued behind it waits. That adds no wait:
the attempt is only sent while the session is active and the connection
is current, so no reconnect is pending, and a rebuild needs the lock,
which the attempt holds until the server answers. A dedicated retry
thread would only add a thread per connection. Nothing else occupies a
MiloClient thread across a service call: the executor only completes the
producer requests and delivers the loss report of a consumer that starts
without a connection; the checks, deletes and resends of the watchdog
and of the session recovery complete on the executor of the Milo client.
- Unchanged: the not-connected result is still synchronous with
lastConnectFailure as its cause; consumer-side failures in whenConnected,
recreateSubscription, recoverSubscription and onTransferFailed still
rebuild the connection; the subscription work keeps the lock and its
ordering.
Behaviour change: a failed write, call, read or browse no longer
disconnects the client and rebuilds the subscriptions of the consumers that
share the connection (no Bad_NoCommunication report, same session).
Producer requests and getOpcUaClient() no longer wait for a consumer that
starts or stops or for the refused-node retry, whose attempts now run on
the MiloClientReconnect thread. While Camel rebuilds the
connection, producers now fail at once with the not-connected result
instead of waiting for the rebuild to finish. Documented in
milo-client-component.adoc (and its catalog copy) and in the 4.24 upgrade
guide; two sentences of the CAMEL-25835 note there that said a failed
request rebuilds the connection are corrected.
Not done (follow-ups):
- handleConnectionFailure still disconnects the old client under the
lock; during a silent drop its CloseSession can block for seconds.
Producers and getOpcUaClient() no longer take the lock, so they no
longer wait for it, and producer failures no longer reach it; only the
subscription work after a consumer-side failure still does. Moving the
disconnect out is not small: whenConnected (under registerItems and
unregisterItems), recreateSubscription and recoverSubscription call it
while they already hold the lock, so they would have to hand the old
connection out of their own lock scope with an identity check, and an
asynchronous disconnect would hold the single MiloClient or reconnect
thread on one processor instead.
- A failed producer request used to re-run endpoint discovery through the
rebuild. Milo's own reconnect reuses the cached endpoint and server
certificate, so a server that renews its certificate or changes its
endpoints while it is unreachable is now recovered only by a Camel
rebuild, for example a context restart. Consumer-only connections
already behaved this way since CAMEL-25691 and CAMEL-25692. Documented
in the component adoc; no mechanism in this commit.
- onTransferFailed still tears down whichever connection is current,
without checking that it is the one whose transfer failed. Pre-existing,
the same class of defect as the producer teardown removed here.
Tests:
- ProducerOnSharedConnectionTest (new):
- producerDoesNotWaitForAConsumerThatStartsOnASilentLink: the relay goes
silent, a consumer starts; once its thread waits in putSubscriptions
for its CreateMonitoredItems call under the lock, getOpcUaClient() and
a fire-and-forget write return within 2 s while the start is still
waiting. On the unmodified code it fails: "execution timed out after
2000 ms", the getOpcUaClient() thread parked in ReentrantLock.lock from
SubscriptionManager.getOpcUaClient.
- writeThatTimesOutKeepsTheConnection: the server holds a write past
requestTimeout=1000; the exchange fails with UaException(Bad_Timeout),
the consumer gets no Bad_NoCommunication and only the next value, same
client and session. On the unmodified code it fails: the consumer
received the Bad_NoCommunication report of the rebuild ("expected: <Bar>
but was: <null>").
- writeOnAReplacedConnectionFailsAndKeepsItsReplacement: a held write is
in flight while a consumer-side failure replaces the connection; the
exchange fails with UaException(Bad_ConnectionClosed) and the
replacement is kept (same client and session, next value only). Passes
on the unmodified code too: there the write fails during the dispose,
before the replacement exists.
- writeDoesNotWaitForTheAttemptToCreateARefusedNodeAgain: a manager
built with a single-thread executor, as with one processor; the server
refuses a node, then holds the next attempt to create it (its attribute
read). A write must complete within 2 s while the attempt waits. It fails
with "execution timed out after 2000 ms" on the unmodified code (the
write parked in ReentrantLock.lock from SubscriptionManager.write) and
with only the lock change, the retry still on the MiloClient executor
(the write waiting in CompletableFuture.get for its outcome).
- TcpRelay gains silence(): the current connections drop their data
without closing, until dropped.
- No existing assertion pinned the producer teardown; only prose did and
is corrected without changing an assertion: MiloClientExecutorTest
(javadoc said a failed request disconnects the client),
MiloClientProducerWriteFailureTest (class javadoc: "while still
triggering the reconnection side-effect"),
SubscriptionManagerExceptionPropagationTest (comment naming
handleConnectionFailure as the side-effect), MiloClientConnection.init()
comments.
- CommunicationLossTest.lostSessionReportsTheNodeAndResendsItsValue fails
under host load with a duplicate resent value on the unmodified code as
well (2 of 5 runs with -Dsurefire.rerunFailingTestsCount=0); unrelated.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The concurrency refactor is sound. The volatile read + final client approach satisfies the Java Memory Model requirements for the lock-free producer path, and the reordering in handleConnectionFailure (setting connected = null before lastConnectFailure) correctly establishes the happens-before relationship that producers need to see a consistent failure cause. One documentation issue in the new test file.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Review note on apache#28371: the javadoc of assertStatus() was left in front of the fourth test when the test was added. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the orphaned Javadoc finding is fully addressed: assertStatus() now has its proper Javadoc at the correct location in ProducerOnSharedConnectionTest.java.
The concurrency model introduced in this PR is correct:
- The volatile
connectedfield +final OpcUaClient clientinConnectedsatisfies the Java Memory Model requirements for lock-free producer reads. A producer capturing the volatile field into a local variable uses only that reference, and a disposed-connection request fails cleanly with Milo's ownUaException. - The reordering in
handleConnectionFailure()— settinglastConnectFailurebefore nullingconnected— correctly leverages the JMM volatile happens-before chain so thatnewNotConnectedResult()sees the failure cause. outcomeOf()is the right pattern:whenCompleteAsyncwith an empty body shifts completion to the connection's executor without altering the result, so routes never run on Milo's internal transport threads, and a failed request no longer tears down the connection.retryRefused()running entirely onMiloClientReconnectremoves the single-thread executor bottleneck without introducing deadlock —reconnectExecutor.schedule()is a non-blocking enqueue.TcpRelay.silence()is safe:ConcurrentHashMap.newKeySet()provides thread-safe individual operations;synchronized(sockets)protects theArrayListiteration.
Test coverage addresses all four scenarios directly, and each test would fail on unmodified code for the stated reason.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 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.
TL;DR
The change is sound: producers no longer wait for the subscription work, and a failed request no longer tears down a shared connection.
Blockers: None
Other findings: 3, all non-blocking.
Thank you for this careful work. The commit message explains each trade-off, and each new test names what it guards. The change also builds cleanly on #27956 (CAMEL-25694): it keeps whenCompleteAsync, so a failure still reaches the exchange, and it only removes the teardown.
1. Upgrade guide: one consequence is missing
Before this change, a failed request rebuilt the connection, and the rebuild ran endpoint discovery again. So a producer recovered when the server renewed its certificate or changed its endpoints. Now the client reconnects with the endpoint and the certificate that it found at connect time, and nothing rebuilds the connection in that case. The component page says this (milo-client-component.adoc), but the new section in camel-4x-upgrade-guide-4_24.adoc does not. People who upgrade read the upgrade guide, so please add the same point there. For example:
The client reconnects with the endpoint and the server certificate that it found when it connected. If the server changes either while it cannot be reached, the client reaches it again only when Camel builds a new connection, for example when the Camel context restarts.
2. Track the follow-ups in Jira
The commit message names three follow-ups: the disconnect under the lock in handleConnectionFailure(), no rebuild after a server certificate or endpoint change, and no identity check in onTransferFailed. If there are no Jira issues for them yet, please file them, so they are not lost in the commit history. The second one now applies to every connection, not only to consumer-only ones.
3. PR description: test claim
The description says that each new test fails on the unmodified code. The commit message says that writeOnAReplacedConnectionFailsAndKeepsItsReplacement passes there too, because the write fails before the replacement exists. Please correct the description, so reviewers know which tests guard the fix.
Reviewed with Claude Code on behalf of @davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…rtificate consequence The component page already says that the client recovers a lost session with the endpoint and the server certificate it found when it connected, so a server that changes either while it cannot be reached is reached again only when Camel rebuilds the connection. The 4.24 upgrade guide section of this change now says the same, since a failed producer request used to trigger that rebuild. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks @davsclaus. Both corrections are in:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous findings are addressed.
New commit: upgrade guide update (only change in this push)
The sentence added to the 4.24 upgrade guide correctly documents the endpoint/certificate consequence flagged by @davsclaus: the client reconnects with the endpoint and server certificate from its initial connection, so a server that changes either while unreachable is only recovered when Camel rebuilds the connection. The wording is consistent with the component page (milo-client-component.adoc).
No issues with the new documentation. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
The three follow-ups are in Jira, each linked to CAMEL-25912:
|
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 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, 5m 5s total)Total reactor time: 5m 5s
Top 20 slowest modules:
|
Description
Raised in the review of #28212 and filed as CAMEL-25912.
SubscriptionManagerused one lock for the subscription work and for the producer requests. Registering and deleting monitored items, recreating a subscription and the retry of refused nodes (CAMEL-25692) hold that lock across a blocking service call;write,call,readValues,browseandgetOpcUaClient()took the same lock only to read the current connection and start an asynchronous request. A setpoint write therefore waited for unrelated subscription work on the same connection: one round trip normally, about 10 s during a silent network drop (until the keep-alive declared the session lost), up torequestTimeoutwhen the server answered keep-alives but not the call. With one processor the outcome of a producer request also waited for the refused-node retry, which ran its blocking call on the single-threadMiloClientexecutor that completes the producer requests.What changes:
getOpcUaClient()read the current connection without the lock.Connected.clientis final, so a request on a connection disposed meanwhile fails with Milo's ownUaException(Bad_SessionClosed, orBad_ConnectionClosedfor one in flight) instead of aNullPointerException.handleConnectionFailure()anddispose()clear the current connection before they disconnect it, sogetOpcUaClient()keeps returningnullwhile there is no usable connection.MiloClientexecutor, so a route never continues on a Milo thread (CAMEL-25713).MiloClientReconnectthread instead of theMiloClientexecutor. A rebuild needs the lock, which the attempt holds until the server answers, so it does not wait longer than before.lastConnectFailureas its cause; consumer-side failures still rebuild the connection; the subscription work keeps the lock and its ordering.Behaviour change, documented on the component page and in the 4.24 upgrade guide: a failed write, call, read or browse keeps the connection (no
Bad_NoCommunicationreport to the consumers, same session); producer requests andgetOpcUaClient()no longer wait for a consumer that starts or stops or for the retry; while Camel rebuilds the connection a producer request fails at once instead of waiting for the rebuild. Two sentences of the CAMEL-25835 note that said a failed request rebuilds the connection are corrected. Three follow-ups are named in the commit message: the disconnect inhandleConnectionFailure()still runs under the lock; endpoint discovery and a renewed server certificate are now recovered only by a Camel rebuild (as for consumer-only connections since CAMEL-25691);onTransferFailedhas no identity check.Tests: new
ProducerOnSharedConnectionTest(4 tests on the embedded server and the TCP relay, which gainssilence()): a producer andgetOpcUaClient()return within 2 s while a consumer start waits under the lock on a silent link; a write that times out keeps the connection; a write on a replaced connection fails withBad_ConnectionClosedand keeps the replacement; a write completes within 2 s while the retry stalls on a single-thread executor. The first, second and fourth fail on the unmodified code for the stated reason; the third passes there too, because the write fails during the dispose, before the replacement exists, so it guards the kept replacement rather than the removed teardown. No existing assertion pinned the old teardown. Module-Psourcecheck verifywith-Dsurefire.rerunFailingTestsCount=0: 125 tests, 0 failures, 1 skipped (as upstream).Measured on a real server (opc-plc 2.15.4 and node-opcua, client with
--cpus=1, two runs each):getOpcUaClient()wait on the lockBad_NoCommunicationper consumerFixes CAMEL-25912.
@davsclaus @oscerd
Target
mainbranch)Tracking
Apache Camel coding standards and style
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