CAMEL-23902: Fix flaky UndertowWsConsumerRouteTest#24421
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: 37 tested, 29 compile-only — current: 37 all testedMaveniverse Scalpel detected 66 affected modules (current approach: 37).
|
The WebSocket tests in UndertowWsConsumerRouteTest fail intermittently on JDK 17 because WebSocket.sendText() returns a CompletableFuture that must complete before the next send. When two messages are sent in rapid succession, the second send can fail silently with IllegalStateException. Replace CountDownLatch-based assertTrue(await(10)) with Awaitility polling in echo(), echoMulti(), sendToAll() and connectionKeyList(). For tests that send multiple messages on the same connection (echo, connectionKeyList), wait for the echo of the first message before sending the next to avoid the JDK WebSocket pending-send conflict. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
davsclaus
force-pushed
the
camel-23902-fix-flaky-test-undertowwsconsumerrout
branch
from
1b2b35d to
1f0f1ed
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to stabilize UndertowWsConsumerRouteTest on JDK 17 by avoiding overlapping WebSocket.sendText() operations and by switching some latch-based waits to Awaitility-based polling.
Changes:
- Add Awaitility to
camel-undertowtest dependencies. - In
echo()andconnectionKeyList(), wait for the first message delivery before sending a second message on the same WebSocket connection. - Replace several
CountDownLatch-basedawait()usages withawait().untilAsserted(...)assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| components/camel-undertow/src/test/java/org/apache/camel/component/undertow/ws/UndertowWsConsumerRouteTest.java | Updates WebSocket tests to sequence sends and to use Awaitility for polling assertions. |
| components/camel-undertow/pom.xml | Adds Awaitility as a test-scoped dependency for the updated tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wsclient1.sendTextMessage("Test1"); | ||
| wsclient1.sendTextMessage("Test2"); | ||
|
|
||
| assertTrue(wsclient1.await(10)); | ||
| // Wait for the first echo before sending the next message to avoid | ||
| // IllegalStateException from JDK WebSocket when a send is still pending | ||
| await().atMost(10, TimeUnit.SECONDS) | ||
| .untilAsserted(() -> assertTrue(wsclient1.getReceived(String.class).contains("Test1"))); |
There was a problem hiding this comment.
Addressed in commit 01d5f72. Changed the Awaitility polling from untilAsserted() with getReceived(String.class).contains() (which iterates via for-each/iterator) to until(() -> getReceived().size() >= 1) which uses index-based size() — no iterator, safe under concurrent modification. The getReceived(String.class) call is now only made after the size gate confirms all expected messages have arrived and the listener thread is done mutating the list.
| await().atMost(10, TimeUnit.SECONDS) | ||
| .untilAsserted(() -> { | ||
| assertEquals(List.of("Gambas"), wsclient1.getReceived(String.class)); | ||
| assertEquals(List.of("Calamares"), wsclient2.getReceived(String.class)); | ||
| }); |
There was a problem hiding this comment.
Addressed in commit 01d5f72. Replaced untilAsserted() with iterator-based getReceived(String.class) by until(() -> getReceived().size() >= 1) for both clients. The getReceived(String.class) assertions now execute only after the size gate confirms delivery is complete — no concurrent iteration.
| .untilAsserted(() -> { | ||
| List<String> received1 = wsclient1.getReceived(String.class); | ||
| assertEquals(2, received1.size()); | ||
| assertTrue(received1.contains("Gambas")); | ||
| assertTrue(received1.contains("Calamares")); |
There was a problem hiding this comment.
Addressed in commit 01d5f72. The Awaitility polling now uses until(() -> getReceived().size() >= 2) for both clients (no iterator). The getReceived(String.class) list and contains() checks are only called after the size gate confirms both messages have arrived, so no concurrent modification risk.
| await().atMost(10, TimeUnit.SECONDS) | ||
| .untilAsserted(() -> assertConnected(wsclient1)); | ||
| final String connectionKey1 = assertConnected(wsclient1); | ||
| assertNotNull(connectionKey1); | ||
| wsclient2.await(10); | ||
| await().atMost(10, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Addressed in commit 01d5f72. The Awaitility polling now uses until(() -> getReceived().size() >= 1) — no iterator. assertConnected() (which calls getReceived(String.class).get(0)) is only invoked after the size gate confirms the CONNECTED message has been received, so the listener thread is done mutating the list at that point.
| await().atMost(10, TimeUnit.SECONDS) | ||
| .untilAsserted(() -> { | ||
| assertEquals(broadcastMsg, wsclient2.getReceived(String.class).get(0)); | ||
| assertEquals(broadcastMsg, wsclient3.getReceived(String.class).get(0)); | ||
| }); |
There was a problem hiding this comment.
Addressed in commit 01d5f72. Both the broadcast and private message waits now use until(() -> getReceived().size() >= 1) for polling. The getReceived(String.class).get(0) content assertion is only made after the size gate confirms delivery — safe from both ConcurrentModificationException and IndexOutOfBoundsException.
…polling Address Copilot review: getReceived(String.class) iterates over WebsocketTestClient's non-thread-safe ArrayList via for-each, risking ConcurrentModificationException while the listener thread adds messages. Switch Awaitility polling to use getReceived().size() (index-based, no iterator). Content assertions via getReceived(String.class) are only called after the size check confirms all expected messages arrived. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Claude Code on behalf of Guillaume Nodet
Fix flaky WebSocket tests in
UndertowWsConsumerRouteTestthat fail intermittently on JDK 17.Root cause: JDK's
WebSocket.sendText()returns aCompletableFuturethat must complete before the next send. When two messages are sent in rapid succession on the same connection, the second send fails silently withIllegalStateExceptionbecauseWebsocketTestClient.sendTextMessage()ignores the returned future.Fix:
CountDownLatch-basedassertTrue(await(10))+ subsequent assertions with Awaitility polling inecho(),echoMulti(),sendToAll(), andconnectionKeyList()getReceived().size()(index-based, no iterator) to avoidConcurrentModificationExceptionon the non-thread-safeArrayListduring concurrent polling. Content assertions viagetReceived(String.class)are only called after the size check confirms all expected messages arrived.echo,connectionKeyList), wait for the echo of the first message before sending the next, avoiding the JDK WebSocket pending-send conflictawaitilitytest dependency tocamel-undertowTest plan
UndertowWsConsumerRouteTestpasses (all 10 tests, 0 flakes)echo()test 3x consecutively — all greenmvn formatter:format impsort:sort— no changes needed)🤖 Generated with Claude Code