Repository navigation
CAMEL-25713: camel-milo - give each client connection its own executor - #28049
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Well-designed fix for a real deadlock on single-CPU containers. By giving each MiloClientConnection its own ScheduledExecutorService, the fix correctly decouples Milo's internal tasks from the Camel route's connect()/close() lifecycle — eliminating the deadlock without affecting shared-executor users.
One low-severity defensive coding issue flagged below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the lock leak issue has been addressed.
The previous finding flagged lock.unlock() being called after executor.shutdown() in the finally block, which could leave the lock permanently held if shutdown threw. The new commit wraps the executor shutdown in its own inner try/finally, guaranteeing lock.unlock() runs regardless. ✅
Pre-fix test validation: awaitedWriteCompletesWhileTheSharedScheduledExecutorOfMiloIsBusy correctly exercises the deadlock scenario — blocking all threads of Milo's shared executor, then doing an awaited write that would hang indefinitely without the per-connection executor. executorIsShutDownWithTheConnection validates cleanup. Both tests would fail on the original code.
No new issues found. 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.
Thanks @etzel-mes. I traced the lifecycle of the new pool. init() creates it lazily under the connection lock, from the ExecutorServiceManager of the CamelContext, so the context also shuts it down when it stops. close() now shuts it down under the same lock, even when dispose() fails. MiloClientCachingConnectionManager closes the connection when its last producer or consumer releases it, so stopping and starting a route gives a new connection with a new pool. The Abort policy behaves as described: when the pool is shut down, CompletableFuture completes the dependent stage exceptionally with the RejectedExecutionException. With CallerRuns, the pool would drop the task silently. Both endpoints set the CamelContext on every createConnection(). The thread name leaves out the cache id, which contains the endpoint URI. The new test follows the JUnit style of the module and has no sleeps.
gnodet-bot's lock-leak point is fixed in 928fac4, and that thread is already resolved. The PR merges cleanly with #28045 and #27956 (checked with git merge-tree).
I left one non-blocking comment inline about how many threads each connection now gets. Approving.
Claude Code on behalf of Claus Ibsen
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 8 of 706 tested, 26 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (2 direct + 7 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (26)
All tested modules (35 modules, 5m 52s total)Total reactor time: 5m 52s
Top 20 slowest modules:
|
A client connection handled the outcome of its write, method call, read and browse requests, and ran its reconnect job, on the shared scheduled executor of Milo. A failed request disconnects the client, and the CloseSession of the disconnect can wait for a reconnect attempt of the channel, which Milo schedules on that same executor. The executor has as many threads as the JVM has processors, so with one processor its only thread waited for a task that only it could run: the connection was not rebuilt, its consumers stayed silent, and every call that needs the lock of the connection waited. Each connection now has a scheduled thread pool of its own from the ExecutorServiceManager of the CamelContext, with up to as many threads as the shared scheduled executor of Milo, and shuts it down when it is closed. Like the queue of that executor, its queue is not bounded, because a rejected reconnect would end the reconnects of the connection. A request that completes after the connection is closed fails with a RejectedExecutionException instead of being dropped. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…l fails to shut down close() released the lock of the connection at the end of the finally block that shuts down its thread pool, so an exception from the shutdown left the lock held. The lock now has a finally block of its own, and the pool is still shut down when disposing the connection fails. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
928fac4 to
b065630
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Previous lock-leak finding was addressed in 928fac4. The commit b065630 restructures close() again, this time with a properly nested try-finally that guarantees lock.unlock() is reached even if ExecutorServiceManager.shutdown() throws — correct.
Code review of b065630:
The fix itself is sound. Each MiloClientConnection now owns a dedicated ScheduledExecutorService obtained from Camel's ExecutorServiceManager, breaking the shared-executor deadlock on single-CPU containers. Key observations:
-
close()resource cleanup — The nested try-finally correctly ensures: (a)manager.dispose()runs first, (b)executor.shutdown()runs even if dispose throws, (c)lock.unlock()runs even if shutdown throws. ✅ -
Thread safety —
camelContextisvolatile,executoris only written underlock, andcheckInit()is guarded by the same lock. The only potential concern is thatclose()does not resetinitializedtofalse, but that's a pre-existing pattern (the old code also nulledmanagerinclose()without resetting the flag), and the connection is not expected to be reused after close. Acceptable. -
init()pool configuration —poolSize = availableProcessors()with unbounded queue andAbortrejection policy. The pool sizing was already noted by @davsclaus as a future optimization opportunity (many connections × many processors = many idle threads). Not a blocker. -
CamelContextAwareintegration — BothMiloBrowseEndpointandMiloClientEndpointnow inject theCamelContextinto the connection after creation. The upgrade guide documents the newsetCamelContextrequirement for externally-createdMiloClientConnectioninstances. ✅ -
Tests —
awaitedWriteCompletesWhileTheSharedScheduledExecutorOfMiloIsBusydirectly exercises the deadlock scenario by saturating Milo's shared executor with blocking tasks, then verifying a write still completes (it uses the connection's own pool).executorIsShutDownWithTheConnectionverifies thread cleanup. Both are well-targeted. ✅
No issues found on this revision.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @etzel-mes. Since my approval the branch was rebased on main. git range-diff shows both commits unchanged; only the context of the upgrade-guide entry moved, because other 4.24 entries landed above it. The branch still merges cleanly with main. CI for the new head was just approved and is running.
My inline note about how many threads each connection gets is still open. It does not block this PR (the upgrade guide covers it), but please reply to it or resolve it when you've read it. Approval stands.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
Closing and reopening to re-run CI against main: the Claude Code on behalf of Claus Ibsen |
Description
CAMEL-25713.
A
milo-clientormilo-browseconnection handled the outcome of its write, method call, read and browse requests, and ran its reconnect job, onStack.sharedScheduledExecutor(), the scheduled executor that Milo shares between all its clients. When a request fails,SubscriptionManager.handleConnectionFailureholds the lock of the connection and callsOpcUaClient.disconnect(). A CloseSession that is sent after the client noticed the lost connection waits until a reconnect attempt restores the channel or fails, and Milo schedules those attempts on that same executor. That executor has as many threads as the JVM has processors, so with one processor its only thread waits for a task that only it could run.disconnect()waits without a timeout, and the CloseSession starts its request timeout only once it has a channel, so nothing ends the wait: the connection is not rebuilt, its consumers stay silent after the server can be reached again, and every call that needs the lock of the connection waits.OPC UA clients often run on small gateways or in containers and pods, and a CPU limit of one or less gives the JVM one processor. For the issue, against Microsoft opc-plc 2.15.4 with the client limited to one CPU, a producer writing every 2 s and the traffic to the server dropped for 30 s, main (a16cccb) did not rebuild the connection within 60 s in 3 of 11 runs, each time with
milo-shared-scheduled-executor-0waiting inOpcUaClient.disconnect().The change:
MiloClientConnectionnow has a scheduled thread pool of its own, created by theExecutorServiceManagerof theCamelContextand shut down when the connection is closed.MiloClientEndpointandMiloBrowseEndpointgive the connection theirCamelContext. Milo still schedules its reconnect attempts on its shared scheduled executor, which a disconnect no longer blocks.Abort: a request that completes after the connection is closed fails with aRejectedExecutionException. With the defaultCallerRuns, a pool that is shut down drops the task, and the request would never complete.close()takes the lock thatinit()runs under, and shuts the pool down even if disposing the connection fails.SubscriptionManageris unchanged apart from one sentence of a Javadoc that said the two executors were the same.MiloClientExecutorTest: an awaited write completes while every thread of Milo's shared scheduled executor is busy, and the threads of the pool end when the last route that uses the connection stops. Without the change both tests fail: the write times out, and no thread of the pool exists. The first test shows that the outcome of a request no longer needs a thread of Milo's executor; it does not lose the connection.With one processor, or with virtual threads enabled (the
ExecutorServiceManagerthen builds a scheduled pool with one worker), the pool has a single thread. A route that runs on it, after an awaited request or for the status of a monitored item that a reconnect could not create, must then not wait for another request of the same connection, for example through aProducerTemplate. With one processor, Milo's shared scheduled executor had the same limit.Calling
disconnectAsync()inhandleConnectionFailurewould avoid this one wait, but the reconnect job blocks inconnect()until the server answers or the connect times out, and on Milo's shared scheduled executor that delays the timers of every Milo client in the JVM.The issue mentions that handling the failures on another executor could need a guard against a stale failure starting a second reconnect. The pool never has more threads than Milo's shared scheduled executor, so it adds no such race: with one thread, failures that complete during the disconnect run before the queued reconnect and find it already scheduled; with more threads, the race is the one that main has. A guard needs the failing connection in the four producer blocks that #27956 changes, so it fits better after that PR. CAMEL-25692 proposes an executor owned by the component for the subscription recovery that #27910 added; this pool could serve as that executor.
Against the same server, main (e5ae19c) and this change ran side by side, each with a consumer and an awaited producer on one connection, and each with its traffic dropped for 30 s. With half a CPU per client (the JVM still sees one processor), main did not rebuild the connection within 60 s in 4 of 12 runs, each time with
milo-shared-scheduled-executor-0waiting inOpcUaClient.disconnect()fromhandleConnectionFailureof a write; with the change, the connection was rebuilt in all 12. With one CPU per client, both rebuilt it in all 18 runs: the hang is a race, and there the CloseSession of main always failed at once. In 6 of the 30 runs the CloseSession of the change waited about 6 s, until a reconnect attempt of the channel failed while the traffic was still dropped; on main that wait does not end.This PR does not change the lines that #27956 (CAMEL-25694, @gnodet) and #28045 (CAMEL-25690) change. It merges with each of them without conflicts in either order, and all three merge together; on top of #27956, the new test and the tests of #27956 pass.
mvn -Psourcecheck clean verifyincomponents/camel-milopasses (81 tests, 1 skipped as on main), and the module build regenerates no files. The module tests passed three more times with-Dsurefire.rerunFailingTestsCount=0, once with-XX:ActiveProcessorCount=1.@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