CAMEL-24789: camel-vertx-websocket - finish an exchange with no peer synchronously - #26541
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
🔄 Backport BotThis bugfix targets
Labels Port PRs will be created automatically when this PR is merged. Comment ℹ️ If you push additional commits after |
gnodet-bot
left a comment
There was a problem hiding this comment.
The production fix is correct — callback.done(true) now matches return true, closing the AsyncProcessor contract violation. The unmatched-key WARN closes the silent-drop gap in getConnectedPeers. Two test gaps worth closing.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 of 692 tested, 27 compile-only — current: 10 all testedMaveniverse Scalpel detected 37 affected modules (current approach: 10). Skip-tests mode would test 10 modules (1 direct + 9 downstream), skip tests for 27 (generated code, meta-modules)
|
| Module | Duration | Status |
|---|---|---|
| Camel :: Launcher | 51.5s | SUCCESS |
| Camel :: JBang :: MCP | 41.3s | SUCCESS |
| Camel :: Vert.x :: WebSocket | 34.8s | SUCCESS |
| Camel :: JBang :: Plugin :: TUI | 34.1s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 23.6s | SUCCESS |
| Camel :: Component DSL | 21.0s | SUCCESS |
| Camel :: JBang :: Plugin :: Kubernetes | 16.0s | SUCCESS |
| Camel :: Docs | 15.1s | SUCCESS |
| Camel :: YAML DSL :: Validator | 10.0s | SUCCESS |
| Camel :: Catalog :: Camel Report Maven Plugin | 9.8s | SUCCESS |
| Camel :: Kamelet Main | 8.4s | SUCCESS |
| Camel :: JBang :: Plugin :: Testing | 7.7s | SUCCESS |
| Camel :: Catalog :: Camel Route Parser | 7.7s | SUCCESS |
| Camel :: IBM :: watsonx.ai | 5.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Validate | 5.5s | SUCCESS |
| Camel :: All Components Sync point | 5.4s | SUCCESS |
| Camel :: YAML DSL :: Deserializers | 5.3s | SUCCESS |
| Camel :: YAML DSL :: Validator Maven Plugin | 3.7s | SUCCESS |
| Camel :: Catalog :: Maven | 3.4s | SUCCESS |
| Camel :: YAML DSL :: Maven Plugins | 3.1s | SUCCESS |
| Camel :: Catalog :: Suggest (deprecated) | 2.3s | SUCCESS |
| Camel :: Coverage | 1.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Edit | 1.7s | SUCCESS |
| Camel :: Endpoint DSL :: Support | 1.3s | SUCCESS |
| Camel :: Assembly | 1.2s | SUCCESS |
| Camel :: JBang :: Integration tests | 1.1s | SUCCESS |
| Camel :: Catalog :: Dummy Component | 0.9s | SUCCESS |
| Camel :: JBang :: Plugin :: Generate | 0.9s | SUCCESS |
| Camel :: Catalog :: Console | 0.8s | SUCCESS |
| Camel :: JBang :: Main | 0.7s | SUCCESS |
| Camel :: Launcher :: Container | 0.7s | SUCCESS |
| Camel :: JBang :: Plugin :: MCP | 0.7s | SUCCESS |
| Camel :: JBang :: Plugin :: Route Parser | 0.5s | SUCCESS |
| Camel :: Endpoint DSL | n/a | |
| Camel :: Integration Tests | n/a | |
| Camel :: JBang :: Core | n/a | |
| Camel :: YAML DSL | n/a |
Top 20 slowest modules:
Camel :: Launcher(51.5s)Camel :: JBang :: MCP(41.3s)Camel :: Vert.x :: WebSocket(34.8s)Camel :: JBang :: Plugin :: TUI(34.1s)Camel :: Catalog :: Camel Catalog(23.6s)Camel :: Component DSL(21.0s)Camel :: JBang :: Plugin :: Kubernetes(16.0s)Camel :: Docs(15.1s)Camel :: YAML DSL :: Validator(10.0s)Camel :: Catalog :: Camel Report Maven Plugin(9.8s)Camel :: Kamelet Main(8.4s)Camel :: JBang :: Plugin :: Testing(7.7s)Camel :: Catalog :: Camel Route Parser(7.7s)Camel :: IBM :: watsonx.ai(5.8s)Camel :: JBang :: Plugin :: Validate(5.5s)Camel :: All Components Sync point(5.4s)Camel :: YAML DSL :: Deserializers(5.3s)Camel :: YAML DSL :: Validator Maven Plugin(3.7s)Camel :: Catalog :: Maven(3.4s)Camel :: YAML DSL :: Maven Plugins(3.1s)
…synchronously With no peer to send to, the producer completed its callback with doneSync=true and then returned false, which tells the caller the opposite: that the exchange will be finished later, from a write handler that never runs because there is nothing to write to. AsyncProcessor#process documents the return value as doneSync, and the null-body branch a few lines earlier in the same method already gets this right. The empty case now completes and returns true, and says at WARN that the message was not delivered. The path is easy to reach: sendToAll with nothing connected yet, or a CamelVertxWebsocket.connectionKey that matches no current peer. A connection key that matches nothing was also dropped in silence, while a peer whose websocket turned out to be null one line further on did warn. Both report now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
camel-vertx-websocket is written against org.junit.jupiter.api.Assertions in all of its other test classes, so the new one follows suit rather than arriving as the only AssertJ file, which also takes the assertj test dependency back out. Same point davsclaus raised on CAMEL-24788. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
ec4e01e to
b59fc18
Compare
|
Rebased onto current main (which now has #26533) and applied the house-style point davsclaus made there, before it had to be made twice: the test is written with Re-checked that the test still catches the bug after the conversion: with the producer change reverted it fails on the contract itself — 91 module tests green, full reactor green. Claude Code on behalf of oscerd |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after the rebase. The house-style conversion (assertj → JUnit) is done. Two findings from the previous review remain open.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…o-peer tests anExchangeWithNoBodyIsDoneSynchronously asserted only that process() returned true and that doneSync was true, and a callback completed twice satisfies both. It now counts callbacks like its sibling does, so a double-invocation fails. The unmatched connection-key path had no test at all. It is only reachable when peers exist and the key matches none of them, which needs a real consumer - peers are added from a live connection handler, so the producer unit test cannot get there. The test therefore lives in VertxWebsocketTest, where the server harness already is: one connected peer, a key nobody has, and an assertion that the peer receives nothing. That pins behaviour rather than the log line, which would need a log appender to assert and would be brittle. It is the behaviour that matters: mutating the branch to fall back to putAll(peers) - the plausible wrong fix - makes the test fail, so an unmatched key silently fanning out to whoever is connected is now caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after the third push.
Both findings from the previous CHANGES_REQUESTED are addressed:
Finding 1 — callback-count assertion in anExchangeWithNoBodyIsDoneSynchronously: Fixed. The test now uses an AtomicInteger and asserts assertEquals(1, callbacks.get()), matching the pattern in the first test. A second callback.done(true) invocation would now be caught.
Finding 2 — no unit test for the unmatched CONNECTION_KEY path: Addressed in VertxWebsocketTest.sendWithAnUnmatchedConnectionKeyDeliversToNobody. Oscerd's explanation of why it cannot live in VertxWebsocketProducerNoPeerTest is correct — the getConnectedPeers branch that logs the WARN requires peers to be non-empty, which is only reachable with a live server; an empty registry falls to the external-server else branch instead. The integration test with a real connected peer and an unmatched key covers the observable contract: the send returns without delivering to anyone.
Production fix and both test gaps are in order. CI is green. APPROVE.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
LGTM. callback.done(true) followed by return false was a genuine AsyncProcessor contract violation and the fix mirrors the null-body branch just above it; the result handler is now only built when there is someone to write to. The two review items from the earlier round are addressed in 8b95506d, and the placement of the unmatched-key test in VertxWebsocketTest is right - with an empty registry that path falls through to the outbound-connection branch, so it needs a real peer. Verified locally: 92/92 tests pass in camel-vertx-websocket on this branch.
One non-blocking thought on log level inline.
Claude Code on behalf of @davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Having no peer connected is an ordinary state for a broadcast - a route pushing status on a timer before any browser attaches hits it on every tick - so warning each time turns a normal condition into log noise. That line drops to debug. The misconfiguration keeps its warning: a connection key that matches no peer is still reported at WARN from getConnectedPeers, which is the case where the route asked for a specific peer and did not get it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after the fourth commit (8a77a8f).
The change is correct: LOG.warn → LOG.debug for the empty-peers broadcast case, exactly as davsclaus suggested. A status route firing before any browser attaches hits this on every tick, so warning each time is log noise — debug is the right level. The misconfiguration path (explicit CONNECTION_KEY matching nothing) keeps its LOG.warn in getConnectedPeers, which is where it belongs.
All findings from the previous three review rounds are addressed. Production fix, both test gaps, and the log-level calibration are in order. CI is green (37 modules, all SUCCESS).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after commit 8a77a8f.
The only change in this commit is lowering the empty-peer broadcast log line from LOG.warn to LOG.debug. The reasoning is correct: a route that broadcasts on a timer before any browser connects hits this path on every tick, and a WARN per tick is log noise for a normal operating condition. The targeted-key-not-found case correctly keeps its LOG.warn in getConnectedPeers — that one is a genuine misconfiguration signal.
All prior findings remain addressed. CI green. APPROVE.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
🔄 Backport BotPort PRs created:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
6 similar comments
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
28 similar comments
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
🔄 Backport BotPort PRs updated:
ℹ️ If you push additional commits after |
Found by the same source audit of
components/camel-vertxas #26533.The problem
VertxWebsocketProducer.processtold its callback the exchange had finished synchronously and told thecaller it had not:
AsyncProcessor#processdocuments the return value asdoneSync— "true to continue executionsynchronously, false to continue being executed asynchronously" — so
falsesays the exchange will befinished later, by a write handler that never runs because there is nothing to write to. The null-body
branch a few lines earlier in the same method already does it correctly (
callback.done(true); return true;).Easy to reach:
sendToAllwith nothing connected yet, or aCamelVertxWebsocket.connectionKeythat matchesno current peer.
The change
The empty case completes the callback, returns
true, and logs a WARN naming the endpoint, so a messagethat went nowhere is visible rather than silent.
A connection key that matches nothing was also dropped without a word, by
.filter(peers::containsKey),while a peer whose websocket turned out to be null one line further on did warn. Both report now.
Tests
VertxWebsocketProducerNoPeerTest(new, 2 cases) asserts what the contract is about — the valueprocessreturns, the single callback, and its
doneSyncflag — for a broadcast that reaches no peer and for a nullbody. The endpoint is built directly so nothing opens a connection.
Confirmed the test catches this: with the original completion handling restored,
anExchangeWithNoPeerIsDoneSynchronouslyfails on the return value (Expecting value to be true but was false).mvn clean install -DskipITsoncomponents/camel-vertxis green — 87 tests. Full reactormvn clean install -DskipTests -DskipITs -Dquicklygreen.org.assertj:assertj-corewas not on this module's test classpath and is added; #26533 adds the same line,so whichever merges second will drop that hunk.
Claude Code on behalf of oscerd
🤖 Generated with Claude Code