Copilot commented on code in PR #24421:
URL: https://github.com/apache/camel/pull/24421#discussion_r3524539487
##########
components/camel-undertow/src/test/java/org/apache/camel/component/undertow/ws/UndertowWsConsumerRouteTest.java:
##########
@@ -207,19 +212,18 @@ public void sendToAll() throws Exception {
wsclient1.sendTextMessage("Gambas");
wsclient2.sendTextMessage("Calamares");
- assertTrue(wsclient1.await(10));
- assertTrue(wsclient2.await(10));
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> {
+ List<String> received1 =
wsclient1.getReceived(String.class);
+ assertEquals(2, received1.size());
+ assertTrue(received1.contains("Gambas"));
+ assertTrue(received1.contains("Calamares"));
Review Comment:
This Awaitility block repeatedly iterates over WebsocketTestClient's
internal ArrayList via getReceived(String.class), which is mutated by the
WebSocket listener thread. That can throw ConcurrentModificationException
during polling retries. Consider asserting using getReceived() with size/index
checks, and derive a Set from the two elements to avoid order sensitivity
without iterating.
##########
components/camel-undertow/src/test/java/org/apache/camel/component/undertow/ws/UndertowWsConsumerRouteTest.java:
##########
@@ -187,11 +192,11 @@ public void echoMulti() throws Exception {
wsclient1.sendTextMessage("Gambas");
wsclient2.sendTextMessage("Calamares");
- assertTrue(wsclient1.await(10));
- assertTrue(wsclient2.await(10));
-
- assertEquals(List.of("Gambas"), wsclient1.getReceived(String.class));
- assertEquals(List.of("Calamares"),
wsclient2.getReceived(String.class));
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> {
+ assertEquals(List.of("Gambas"),
wsclient1.getReceived(String.class));
+ assertEquals(List.of("Calamares"),
wsclient2.getReceived(String.class));
+ });
Review Comment:
Awaitility polling currently calls getReceived(String.class), which iterates
over WebsocketTestClient's non-thread-safe ArrayList while its listener thread
is adding messages. This can introduce new flakiness (e.g.,
ConcurrentModificationException). Prefer size/index checks against
getReceived() (no iterator) during polling.
##########
components/camel-undertow/src/test/java/org/apache/camel/component/undertow/ws/UndertowWsConsumerRouteTest.java:
##########
@@ -168,11 +169,15 @@ public void echo() throws Exception {
wsclient1.connect();
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")));
Review Comment:
Using wsclient1.getReceived(String.class) inside Awaitility polling iterates
over WebsocketTestClient's internal ArrayList while its listener thread is
concurrently adding messages, which can lead to ConcurrentModificationException
or inconsistent reads during retries. A safer pattern here is to poll using
getReceived() with size/index checks (no iterator) and only then assert message
ordering/content.
##########
components/camel-undertow/src/test/java/org/apache/camel/component/undertow/ws/UndertowWsConsumerRouteTest.java:
##########
@@ -292,27 +296,34 @@ public void connectionKeyList() throws Exception {
wsclient2.connect();
wsclient3.connect();
- wsclient1.await(10);
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertConnected(wsclient1));
final String connectionKey1 = assertConnected(wsclient1);
assertNotNull(connectionKey1);
- wsclient2.await(10);
+ await().atMost(10, TimeUnit.SECONDS)
Review Comment:
The new Awaitility polling calls assertConnected(), which uses
getReceived(String.class) and iterates over WebsocketTestClient's
non-thread-safe ArrayList while the listener thread is adding the CONNECTED
message. Polling via iterator increases the chance of
ConcurrentModificationException or visibility issues. Consider polling using
getReceived() with size/index checks and only then extracting the connection
key.
##########
components/camel-undertow/src/test/java/org/apache/camel/component/undertow/ws/UndertowWsConsumerRouteTest.java:
##########
@@ -292,27 +296,34 @@ public void connectionKeyList() throws Exception {
wsclient2.connect();
wsclient3.connect();
- wsclient1.await(10);
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertConnected(wsclient1));
final String connectionKey1 = assertConnected(wsclient1);
assertNotNull(connectionKey1);
- wsclient2.await(10);
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertConnected(wsclient2));
final String connectionKey2 = assertConnected(wsclient2);
- wsclient3.await(10);
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertConnected(wsclient3));
final String connectionKey3 = assertConnected(wsclient3);
wsclient1.reset(1);
wsclient2.reset(1);
wsclient3.reset(1);
final String broadcastMsg = BROADCAST_MESSAGE_PREFIX + connectionKey2
+ " " + connectionKey3;
wsclient1.sendTextMessage(broadcastMsg); // this one should go to
wsclient2 and wsclient3
- wsclient1.sendTextMessage("private"); // this one should go to
wsclient1 only
+ // Wait for broadcast delivery before sending the next message to avoid
+ // IllegalStateException from JDK WebSocket when a send is still
pending
+ await().atMost(10, TimeUnit.SECONDS)
+ .untilAsserted(() -> {
+ assertEquals(broadcastMsg,
wsclient2.getReceived(String.class).get(0));
+ assertEquals(broadcastMsg,
wsclient3.getReceived(String.class).get(0));
+ });
Review Comment:
These Awaitility assertions call getReceived(String.class).get(0), which (a)
iterates the underlying non-thread-safe ArrayList, and (b) can throw
IndexOutOfBoundsException while messages are still in flight. To keep polling
resilient, assert on getReceived() size first and then use index-based access
for the first message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
