orpiske commented on code in PR #24247:
URL: https://github.com/apache/camel/pull/24247#discussion_r3474273829
##########
core/camel-core/src/test/java/org/apache/camel/component/seda/FileSedaShutdownCompleteAllTasksTest.java:
##########
@@ -52,16 +52,9 @@ public void configure() {
.shutdownRunningTask(ShutdownRunningTask.CompleteAllTasks).to("log:delay").to("seda:foo");
from("seda:foo").routeId("route2").to("log:bar").to("mock:bar").process(new
Processor() {
- boolean first = true;
-
@Override
public void process(Exchange exchange) throws Exception {
Review Comment:
**[Important — Test semantics changed]**
The removed `Thread.sleep(100)` (and the `first` boolean) simulated a slow
processor on the first message. The test's purpose is to verify that shutdown
completes *all* tasks even when one is in-flight and slow. Without the sleep,
all 5 messages process near-instantly and this scenario is no longer exercised.
This sleep was load-bearing for the test's intent, not a synchronization
artifact. If the goal is to avoid `Thread.sleep`, consider using a
`CountDownLatch` that the processor blocks on until the test thread signals it:
```java
CountDownLatch processorGate = new CountDownLatch(1);
// In processor: if (first) { processorGate.await(2, TimeUnit.SECONDS); }
// In test: processorGate.countDown(); then context.stop();
```
##########
core/camel-core/src/test/java/org/apache/camel/component/direct/DirectProducerBlockingTest.java:
##########
@@ -90,6 +94,8 @@ public void run() {
getMockEndpoint("mock:result").expectedMessageCount(1);
+ // Signal that we're about to send (producer will block)
+ producerReady.countDown();
Review Comment:
**[Critical — Race condition]**
`producerReady.countDown()` fires here *before* `template.sendBody()` on the
next line. The background thread can wake up, call `resumeRoute("foo")`, and
complete before `sendBody()` enters its internal blocking loop.
If the route is already resumed when the producer tries to send, the test
passes trivially (send to a running route) rather than testing recovery from a
blocked producer — which was the original test's purpose.
The original `Thread.sleep(200)` was crude but gave `sendBody()` time to
enter the blocking state. The fix needs a signal from *inside* the blocking
logic, or at minimum an observable check (e.g., via Awaitility polling for the
producer thread being in `WAITING` state or the endpoint showing inflight
exchanges).
If no clean signal point exists, keeping the original sleep with a `// TODO`
comment would be preferable to a latch that doesn't synchronize on the right
event.
##########
core/camel-core/src/test/java/org/apache/camel/processor/aggregator/DistributedCompletionIntervalTest.java:
##########
@@ -36,8 +36,6 @@ public void testCamelContext1Wins() throws Exception {
MockEndpoint mock2 = getMockEndpoint2("mock:result");
mock2.expectedMessageCount(0);
Review Comment:
**[Critical — Timing dependency removed]**
The removed `Thread.sleep(2000)` was ensuring that the aggregator's
completion interval timer (5s / 10s) had started ticking before messages
arrived. This test uses a shared `MemoryAggregationRepository` between two
CamelContexts and relies on the interval timer firing on only one of them.
Without the sleep, message arrival races with timer initialization, which
could cause non-deterministic behavior.
Suggestions:
- Keep the sleep with a comment explaining it's for timer synchronization
(not a synchronization artifact), or
- Use Awaitility to wait for a timer-started signal if one is observable, or
- Restructure the test to combine `completionSize` with `completionInterval`
for determinism.
##########
core/camel-core/src/test/java/org/apache/camel/processor/enricher/PollEnricherNoResourceTest.java:
##########
@@ -39,8 +39,6 @@ public void testNoResourceA() throws Exception {
public void testResourceA() throws Exception {
template.sendBody("seda:foo", "Bye World");
Review Comment:
**[Minor — Subtle behavior change]**
The seda send is asynchronous. The removed `Thread.sleep(250)` ensured the
message was fully enqueued in the seda queue before `pollEnrich("seda:foo",
1000)` started polling. While `pollEnrich` has a 1-second timeout that should
accommodate the async enqueue, the original test was deliberately ensuring the
message was *present* before polling started — a subtly different test scenario
(polling with data already available vs. polling and waiting for data to
arrive).
This is likely safe in practice, but worth acknowledging the intent change.
--
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]
