camel-lumberjack: fix flaky tests by awaiting Netty shutdown#24774
Conversation
- Await shutdownGracefully() on all Netty thread groups in LumberjackServer.stop() so the port is fully released before the next test binds a new server - Await channel.close() and event loop shutdown in test client (LumberjackUtil) to prevent half-open connections from causing spurious ack count mismatches - Make ErrorProcessor.count thread-safe with AtomicInteger in disconnection test - Add explicit 30s Awaitility timeout in LumberjackUtil (was relying on 10s default) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Solid flaky-test fix — all four changes are well-motivated.
-
syncUninterruptibly()on Netty shutdown (LumberjackServer.stop()) — fire-and-forgetshutdownGracefully()is the classic cause of port-still-in-use between@Isolatedtests. Awaiting completion is the standard Netty pattern. -
channel.close().sync()(LumberjackUtil) — ensures the client channel is fully closed before proceeding, preventing half-open connections that cause ack count mismatches. -
AtomicIntegerforErrorProcessor.count— the processor runs on Netty executor threads, so the bareintwas a legitimate race condition. Clean fix. -
Explicit 30s Awaitility timeout — follows the project convention of always setting an explicit
atMostrather than relying on the 10s default. 30s is reasonable for SSL tests on slower CI nodes.
Static analysis note: ast-grep flagged a pre-existing ByteArrayOutputStream without try-with-resources in LumberjackUtil.readSample() (line 112) — not introduced by this PR.
Checklist:
- ✅ Tests: existing tests cover the scenarios; the fix is in resource cleanup, not logic
- ✅ No public API changes
- ✅ Backward compatible — shutdown is just properly awaited now
- ✅ No security concerns
- ⏳ CI: builds in progress
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
| windows.stream().forEach(window -> channel.writeAndFlush(readSample(String.format("io/window%s.bin", window)))); | ||
| if (waitForResult) { | ||
| Awaitility.await().until(() -> windows.size() == responses.size()); | ||
| Awaitility.await().atMost(30, TimeUnit.SECONDS).until(() -> windows.size() == responses.size()); |
There was a problem hiding this comment.
I'm not sure that it is useful given that when things are working the test is taking less than 300 ms
3 participants
Summary
Claude Code on behalf of davsclaus
Fix flaky lumberjack tests caused by incomplete Netty resource cleanup between test runs:
shutdownGracefully()inLumberjackServer.stop(): the three Netty thread groups (bossGroup,workerGroup,executorService) were shut down fire-and-forget, so the server port could still be in use when the next@Isolatedtest tried to bind. Now awaits completion withsyncUninterruptibly().LumberjackUtil:channel.close()was fire-and-forget andshutdownGracefully()on the client event loop was not awaited, which could leave half-open connections that cause spurious ack count mismatches on the server side.ErrorProcessor.count: theint countfield inLumberjackDisconnectionTest.ErrorProcessorwas read/written from Netty executor threads without synchronization. Replaced withAtomicInteger.Awaitility.await()call inLumberjackUtil.sendMessages()relied on the 10-second default, which can be tight on slow CI nodes (especially for SSL tests). Now uses an explicit 30-second timeout.Test plan
mvn testincomponents/camel-lumberjack— all 5 tests pass🤖 Generated with Claude Code