Repository navigation
CAMEL-26112: camel-milo - close a failed connection without waiting for its session to close - #28511
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Clean, well-scoped fix that eliminates a 6–7 s stall on network failure by switching two call sites from synchronous disconnect() to fire-and-forget disconnectAsync(). Thread-safety is maintained by the existing locking protocol, and the new integration test correctly validates the fix.
The refactoring of close() as a shared guard method with dispose()/disposeFailed() as the two disconnect strategies is clean and self-documenting. The Javadoc on disposeFailed() correctly explains why the server won't answer CloseSession on a dead link. The test validates both the non-blocking property (the registering thread never enters OpcUaClient.disconnect()) and functional correctness (connection rebuilt, both consumers receive the current value).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Nice, well-targeted liveness fix. The problem is clear: on the failure paths (handleConnectionFailure, and the catch in performAndEvalConnect) the old code called dispose(), whose synchronous client.disconnect() sends a CloseSession and waits — but on a link that went silent the server never answers, so the call blocks until it times out (or a reconnect attempt fails) while holding lock on the consumer start/stop or reconnect thread, stalling the rebuild. Splitting the "mark closed once" guard into close() and adding disposeFailed() that uses disconnectAsync().whenComplete(...log...) lets a failed connection be torn down without blocking, while the clean path (dispose()) still waits as before. The two call sites are switched to disposeFailed() correctly.
The test is the strong part: failedConnectionIsRebuiltWithoutWaitingForItsSessionToClose silences the relay, triggers the teardown from a registering thread, and inspects that thread's stack to assert it never sits in OpcUaClient.disconnect — i.e. it directly proves the non-blocking path is taken — then verifies the connection is rebuilt (a new client instance) and both consumers resume. @Timeout(60s), Awaitility, no Thread.sleep, and it stays in the file's existing JUnit-assertion style.
Non-blocking:
- Asserting "didn't block" by polling the thread's stack for
OpcUaClient.disconnectis a touch fragile (method-name + timing dependent), though the surrounding "rebuilt without waiting" assertion and the hard timeout make it robust enough in practice. disposeFailed()is fire-and-forget; fine for a failed link being replaced, just noting the async disconnect isn't awaited anywhere.
CI hasn't run here (looks like a fork PR needing a maintainer to trigger it) — worth kicking off before merge.
Claude Code on behalf of oscerd
|
🌟 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 for the careful measurements. Switching the failure paths to disconnectAsync() is the right call. I checked the Milo 1.1.8 jar: disconnect() really is disconnectAsync().get(), and closeSession() only fires an FSM event, so the caller no longer waits. Marking the connection closed before either disconnect, and keeping the waiting dispose() for component stop, both look good.
Two things I'd like to see in this PR:
-
SubscriptionManager.java:1357-1360: the "disposed during connect" case you list under "Not done". When the manager is disposed whileperformConnect()is running, the new connection's client is never disconnected.reconnectJob.cancel(false)doesn't stop a connect that is already running, andMiloClientConnection.close()shuts the executors down gracefully. So a live client with an open session and keep-alive outlives the component. It's a one-liner a few lines above thedisposeFailed()you changed. Could we addconnected.dispose();(ordisposeFailed()) before thatreturn, rather than leave it for a follow-up? -
SubscriptionManager.java:229-234:onTransferFailedcallshandleConnectionFailure(...)without checkingthis.closed.onSessionInactivedoes check it, andrecoverSubscription/recreateSubscriptioncheckisCurrent. With this PR, the failed client keeps running after the rebuild until its CloseSession finishes. In Milo 1.1.8 a CloseSession that arrives while the session is Transferring is shelved until the transfer is done (SessionFsmFactory, Transferring state). If that transfer fails, the old subscription callsonTransferFailed, andhandleConnectionFailurethen tears down whatever connection is current, which may be the replacement. A guard such asif (!this.closed)there (orisCurrent(subscription)underlock) would close this off.
Nit: in disposeFailed() (SubscriptionManager.java:656-660), the whenComplete error branch can't run. Milo 1.1.8 ends disconnectAsync() with .exceptionally(t -> this), so the future always completes normally. Fine to keep as a guard against future Milo versions, but a short comment (or dropping it) would avoid suggesting the failure is logged.
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.
|
Thanks @davsclaus, all three are in 0e55717.
I also moved the disposed-during-connect case out of "Not done" in the description. Claude Code on behalf of etzel-mes |
|
🧪 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 708 tested, 27 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (1 direct + 7 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (27)
All tested modules (35 modules, 4m 31s total)Total reactor time: 4m 31s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Approve — failed connections are now closed with disconnectAsync() without waiting for CloseSession; the follow-up commit addresses all three points from my earlier review.
Blockers
- None
Other findings: none.
Thanks for the quick turnaround. Re-checked at 0e55717:
- Disposed during connect:
performAndEvalConnect()now closes the connection withdisposeFailed(). Nothing waits on the reconnect thread at that point, so the non-waiting close is the right choice, and camel-milo never releases Milo's shared resources on stop, so the async CloseSession still has its threads.connectionCompletedAfterDisposeIsClosedholds the connect in the relay's backlog until afterdispose(), which reproduces it well. onTransferFailednow only tears down underlockwhenisCurrent(subscription), the same checkrecoverSubscriptionuses. Agreed that it's better than!closed, sincedispose()clearsconnectedunder the lock but closes the connection outside it. Fair enough that the test server can't put the old client in Transferring and reject the transfer deterministically.- The dead
whenCompletebranch is gone, and the comment says why.
About the red build (21, false) check: it fails in camel-jbang-core CatalogDocExamplesTest on the camel-hibernate doc examples (selectionQuery "not applicable in producer/consumer only mode"). That isn't from this PR. It came in with the new hibernate component on main and was fixed there by #28530, so a rebase on main should turn it green.
This was a static re-review; it doesn't replace CI or static analysis.
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.
…or its session to close A connection torn down because it failed is now disconnected with OpcUaClient.disconnectAsync(), so the consumer that hit the failure, or the reconnect thread, no longer holds the manager lock and waits for a CloseSession request that the server does not answer on a dead link before the connection is rebuilt. Disposing the manager still waits for the clean disconnect. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ore transfer failures of a replaced connection A connection that performConnect() completes after the manager was disposed is now closed without waiting, since dispose() has already returned without it. A failed subscription transfer now tears down the connection only when the subscription still belongs to the current connection: a client that is being disconnected keeps recovering its session until its CloseSession goes through, and its transfer can fail after it was replaced. The disconnectAsync() future always completes normally, so its unreachable error branch is dropped. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
0e55717 to
5961034
Compare
Description
Follow-up of CAMEL-25912 (#28371). When a consumer-side failure tears the connection down,
handleConnectionFailure()disposed it withOpcUaClient.disconnect(), which sends CloseSession and waits for it, under the manager lock. On a silent network drop the CloseSession is not answered, so the wait lasted 6–7 s on a real server (until the channel's next reconnect attempt failed), and the connection was rebuilt only after it.Measuring it corrected the premise of the issue: no other thread waited on the manager lock for it. The caller is the thread that starts or stops a consumer route, and other route starts and stops in the same CamelContext wait on the context's own route lock, so moving a synchronous disconnect out of the manager lock would not have shortened anything. What helps is not waiting at all.
What changes:
handleConnectionFailure()and the "Failed to set subscriptions" path of the reconnect) is closed with Milo'sOpcUaClient.disconnectAsync()without waiting; a failed disconnect is still logged asFailed to disconnect client. In Milo 1.1.8disconnect()isdisconnectAsync().get(), so this is the same operation without the wait; it runs on Milo's threads, not on theMiloClientexecutor or theMiloClientReconnectthread.Tests: new
CommunicationLossTest.failedConnectionIsRebuiltWithoutWaitingForItsSessionToClose(embedded server behind the TCP relay,requestTimeout=1000): aftersilence(), a registration times out and the registering thread must never be insideOpcUaClient.disconnect; the connection is then rebuilt and both consumers end on the good value. It fails on the unmodified code (connect to failure 6.5 s against 1.3 s) and with onlyhandleConnectionFailure()reverted. Module-Psourcecheck verifywith-Dsurefire.rerunFailingTestsCount=0: 126 tests, 0 failures, 1 skipped (as upstream).Measured on a real server (opc-plc 2.15.4, server and client with
--cpus=1, silent drop with iptables, one CamelContext with consumers, a 50-node consumer route started into the drop, another route stopped and started every 2 s):The rest of a route start into a drop is its CreateMonitoredItems call waiting until the keep-alive declares the session lost, as designed. Reconnect attempts keep the same 15 s cadence; it now starts 6–7 s earlier. In the blip runs both versions leave the old session on the server until its session timeout: the late CloseSession either stays in the dead TCP connection or is rejected with
BadSecureChannelIdInvalidon the new channel, and it never disturbed the new session. Healthy runs: no teardown, no change.A connection that
performConnect()completes after the manager was disposed is now closed too (pre-existing leak, found while testing). CAMEL-26113 and CAMEL-26114 stay separate.Fixes CAMEL-26112.
@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