gnodet-bot commented on code in PR #27616:
URL: https://github.com/apache/camel/pull/27616#discussion_r4246112234
##########
components/camel-lumberjack/src/test/java/org/apache/camel/component/lumberjack/LumberjackUtil.java:
##########
@@ -97,7 +98,17 @@ protected void channelRead0(ChannelHandlerContext ctx,
ByteBuf msg) {
// send 5 frame windows, without pausing
windows.stream().forEach(window ->
channel.writeAndFlush(readSample(String.format("io/window%s.bin", window))));
if (waitForResult) {
- Awaitility.await().atMost(30, TimeUnit.SECONDS).until(() ->
windows.size() == responses.size());
+ Awaitility.await().atMost(30, TimeUnit.SECONDS).untilAsserted(
+ () -> {
+ List<Integer> received;
+ synchronized (responses) {
+ received = new ArrayList<>(responses);
+ }
+ Assertions.assertThat(responses)
+ .as("Expected one ACK per window frame
sent (%d), but received: %s", windows.size(),
+ received)
+ .hasSize(windows.size());
Review Comment:
⚠️ **Thread-safety gap — snapshot taken but assertion reads the live list.**
The `received` snapshot is created under the lock, but
`Assertions.assertThat(responses)` still reads `responses` directly. On every
failed poll Awaitility retries, AssertJ builds the failure message (the `%s`
argument and the actual value) by iterating `responses` while the Netty I/O
thread may be adding to it. This can throw a `ConcurrentModificationException`,
which Awaitility does **not** retry (it only retries `AssertionError`), so the
test would fail with a confusing stack trace. The fix is to assert on
`received` throughout:
```suggestion
Assertions.assertThat(received)
.as("Expected one ACK per window frame
sent (%d), but received: %s", windows.size(),
received)
.hasSize(windows.size());
```
--
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]
