CAMEL-19549: camel-core: replace Thread.sleep in tests#24270
Conversation
Additional improvement to unit test DirectProducerBlockingTest. Introducing better coordination between producer blocking and route resumption, eliminating race conditions on slow hardware. Made with help from AI tools.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Nice follow-up — the prior CountDownLatch approach had a real race condition (countDown() fired before sendBody(), so the route could be resumed before the producer ever started blocking). Polling for TIMED_WAITING ensures the producer is truly blocked inside DirectComponent.getConsumer() before resuming.
Verified that Condition.await(rem, TimeUnit.MILLISECONDS) at DirectComponent.java:144 puts the thread in TIMED_WAITING, so the state check is correct.
Other good changes:
- Error logging instead of silent
// ignore - Mock expectation moved before action
- Timeout increase for slow CI
Minor note: the test is now coupled to the implementation detail that DirectComponent uses Condition.await() (producing TIMED_WAITING). If that blocking mechanism ever changes, this test would need updating. Reasonable tradeoff for a component-specific test.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
orpiske
left a comment
orpiske
left a comment
There was a problem hiding this comment.
Nice follow-up — the CountDownLatch approach on main has a real race (the latch fires before sendBody() is called), and this PR fixes it with a practical Awaitility-based solution. The mock-setup reordering, error logging, and timeout bump are all welcome improvements.
I left a couple of minor observations inline — nothing blocking.
Claude Code on behalf of Otavio Rodolfo Piske. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| await().atMost(2, TimeUnit.SECONDS) | ||
| .pollInterval(10, TimeUnit.MILLISECONDS) | ||
| .until(() -> mainThread.getState() == Thread.State.TIMED_WAITING); | ||
|
|
There was a problem hiding this comment.
Minor observation: TIMED_WAITING is a reliable proxy here (the producer blocks on consumersCondition.await() in DirectComponent.getConsumer), but it's a heuristic — in theory the main thread could transiently enter TIMED_WAITING earlier in the call path before reaching getConsumer(). With the 10ms poll interval this is extremely unlikely in practice, and it's strictly better than the prior CountDownLatch race.
Just something to keep in mind for future maintainers if this test ever becomes flaky again — the Thread.State check is a practical workaround, not a contract.
|
🧪 CI tested the following changed modules:
|
3 participants
Additional improvement to unit test {{DirectProducerBlockingTest}}. Introducing better coordination between producer blocking and route resumption, eliminating race conditions on slow hardware.
Made with help from AI tools.