CAMEL-24126 - Ensure no message sent before the VertxWebsocket is fully configured#24740
CAMEL-24126 - Ensure no message sent before the VertxWebsocket is fully configured#24740apupier wants to merge 1 commit into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 tested, 29 compile-only — current: 10 all testedMaveniverse Scalpel detected 39 affected modules (current approach: 10).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-designed race condition fix — LGTM.
What the PR does
Fixes a race where the WebSocket server could send messages immediately after the handshake completes, but before the consumer's message/close/exception handlers were registered — causing lost messages.
Before: doStart() called getWebSocket() (which blocks until connection completes), then separately called configureWebSocketHandlers(ws). A window existed between connection establishment and handler registration.
After: doStart() calls getWebSocket(this::configureWebSocketHandlers), passing handler setup as a callback. The callback is invoked inside the Vert.x onComplete block before future.complete(ws), so handlers are registered atomically with the connection — no window for lost messages.
The same fix is correctly applied to the reconnection path as well.
Design notes
- The FQCN
java.util.function.Consumer<WebSocket>in the method signature is the right call —org.apache.camel.Consumeris already imported (line 34), so the simple nameConsumerwould be ambiguous. - The Javadoc on the new
getWebSocket(Consumer<WebSocket>)overload clearly explains the atomicity guarantee. - No test is included, which is expected — this is a timing-sensitive race condition that is not reliably reproducible in a unit test. The fix is verifiable by code inspection.
Checklist
- Commit convention — descriptive title
- No public API break —
getWebSocket()isprotected, new overload is additive - No security concerns
- CI — all checks passing (JDK 17, JDK 25)
Static analysis
ast-grep (only tool available) — no findings on the modified files.
Reviewed with Claude on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the contribution @apupier — nice catch on the race condition at the framework level.
The fix looks correct: registering handlers inside the onComplete callback before future.complete(ws) ensures no messages are lost between connection and handler setup. The producer path and reconnect path are both unaffected/correctly updated.
A couple of observations:
-
JIRA ticket — Since this modifies production code (
VertxWebsocketEndpointandVertxWebsocketClientConsumer), it would be good to have a JIRA issue for tracking. The commit message would then follow theCAMEL-XXXX: Brief descriptionconvention. -
Minor error-handling gap — If
setupCallback.accept(ws)were to throw,future.complete(ws)would never be called and the caller would get aTimeoutExceptioninstead of the real cause. In practiceconfigureWebSocketHandlersonly calls handler setters so this is low-risk, but wrapping the callback in a try/catch that callsfuture.completeExceptionally(e)would be more robust.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
| if (setupCallback != null) { | ||
| setupCallback.accept(ws); | ||
| } | ||
| future.complete(ws); |
There was a problem hiding this comment.
Minor robustness suggestion: if setupCallback.accept(ws) throws, future.complete(ws) is never reached and the caller gets a TimeoutException rather than the real cause. Consider:
| future.complete(ws); | |
| if (setupCallback != null) { | |
| try { | |
| setupCallback.accept(ws); | |
| } catch (Exception e) { | |
| future.completeExceptionally(e); | |
| return; | |
| } | |
| } |
Low-risk in practice since configureWebSocketHandlers only calls handler setters, but would help debugging if this pattern is reused elsewhere.
apupier
force-pushed
the
flakyVertxWebsocketTest
branch
from
55c2d3c to
2734888
Compare
configured it was visible with flaky test VertxWebsocketHandshakeHeadersTest.testHandshakeHeadersAsConsumer see rtx-websocket:surefire:test@default-test/details/org.apache.camel.component.vertx.websocket.VertxWebsocketHandshakeHeadersTest/testHandshakeHeadersAsConsumer?top-execution=1 Co-authored-by: IBM Bob IDE 2.0.1 Signed-off-by: Aurélien Pupier <apupier@ibm.com>
apupier
changed the title
apupier
force-pushed
the
flakyVertxWebsocketTest
branch
from
2734888 to
42dd1b5
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for fixing this race condition, @apupier — the approach of registering handlers inside the onComplete callback before future.complete() is clean and correct.
One minor formatting nit inline. Otherwise this looks good.
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
| setupCallback.accept(ws); | ||
| } catch(Exception e) { | ||
| future.completeExceptionally(e); | ||
| return; |
There was a problem hiding this comment.
Minor formatting nit — missing space before the parenthesis:
| return; | |
| } catch (Exception e) { |
3 participants
it was visible with flaky test
VertxWebsocketHandshakeHeadersTest.testHandshakeHeadersAsConsumer see rtx-websocket:surefire:test@default-test/details/org.apache.camel.component.vertx.websocket.VertxWebsocketHandshakeHeadersTest/testHandshakeHeadersAsConsumer?top-execution=1
Co-authored-by: IBM Bob IDE 2.0.1
On main branch, i reproduce the flakiness something like 1 out of 3 times. With this PR, it has never failed in 10 runs
Description
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.