gnodet commented on code in PR #25281:
URL: https://github.com/apache/camel/pull/25281#discussion_r3707644706
##########
core/camel-core/src/test/java/org/apache/camel/component/seda/ThreadPerTaskSedaConsumerTest.java:
##########
@@ -63,6 +67,32 @@ public void testVirtualThreadPerTaskHighThroughput() throws
Exception {
mock.assertIsSatisfied();
}
+ @Test
+ void testShutdownWithConcurrencyLimitCompletesQuickly() throws Exception {
+ // Send messages so the route is actively used
+ for (int i = 0; i < 5; i++) {
+
template.sendBody("seda:limited?virtualThreadPerTask=true&concurrentConsumers=2",
"Message " + i);
+ }
+
+ MockEndpoint mock = getMockEndpoint("mock:limited");
+ mock.expectedMessageCount(5);
+ mock.assertIsSatisfied();
+
+ // Stop the context and verify it completes quickly.
+ // Before the fix, the CountDownLatch was initialized with
concurrentConsumers
+ // count (2) but only 1 coordinator thread counts down, so
prepareShutdown()
+ // would wait the full shutdown timeout before proceeding.
+ long start = System.nanoTime();
+ context.stop();
+ long elapsed = TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() -
start);
+
+ // Shutdown should complete well within the default timeout (300s).
+ // Use a generous 30s bound to avoid flakiness, but this is still much
less
+ // than the full shutdown strategy timeout that would be hit without
the fix.
+ assertTrue(elapsed < 30, "Context stop took " + elapsed + "s, expected
< 30s. "
Review Comment:
**Convention nit:** Per project conventions, new test code should use
AssertJ assertions. Consider:
```java
assertThat(elapsed)
.as("Context stop took %ds; CountDownLatch count likely mismatches
coordinator thread count", elapsed)
.isLessThan(30L);
```
This provides better failure messages and aligns with the project's
preference for AssertJ over JUnit assertions.
##########
components/camel-seda/src/main/java/org/apache/camel/component/seda/SedaConsumer.java:
##########
@@ -49,7 +49,7 @@ public class SedaConsumer extends DefaultConsumer implements
Runnable, ShutdownA
private static final Logger LOG =
LoggerFactory.getLogger(SedaConsumer.class);
private final AtomicInteger taskCount = new AtomicInteger();
- private volatile CountDownLatch latch;
+ protected volatile CountDownLatch latch;
Review Comment:
**Design nit:** Changing `private` → `protected` on the `latch` field
exposes mutable internal state to all subclasses. A template method would
achieve the same goal without field exposure:
```java
// In SedaConsumer:
protected int getLatchCount() {
return getEndpoint().getConcurrentConsumers();
}
// In doStart():
int count = getLatchCount();
if (count > 0) {
latch = new CountDownLatch(count);
}
```
Then `ThreadPerTaskSedaConsumer` just overrides `getLatchCount()` to return
`1` — no need to touch `latch` directly, and it eliminates the "create then
immediately replace" pattern where `super.doStart()` allocates a
`CountDownLatch(N)` that is discarded one line later.
Non-blocking — the current approach is functionally correct.
--
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]
