Raju Gupta created POOL-426:
-------------------------------
Summary: GenericObjectPool does not respect minIdle configuration
Key: POOL-426
URL: https://issues.apache.org/jira/browse/POOL-426
Project: Commons Pool
Issue Type: Bug
Reporter: Raju Gupta
h2. Summary
{{GenericObjectPool.addObject()}} does not respect the {{maxIdle}} limit due to
a race condition, allowing the pool to exceed the configured maximum idle
objects.
*Related:* POOL-425
h2. Root Cause
The {{addObject()}} method (lines 210-221) has a check-then-act race condition:
{code:java}
@Override
public void addObject() throws Exception {
assertOpen();
if (factory == null) {
throw new IllegalStateException("Cannot add objects without a
factory.");
}
final int localMaxTotal = getMaxTotal();
final int localMaxIdle = getMaxIdle();
if (getNumIdle() < localMaxIdle && (localMaxTotal < 0 || createCount.get()
< localMaxTotal)) {
addIdleObject(create(getMaxWaitDuration()));
}
}
{code}
*Problem:* The check {{getNumIdle() < localMaxIdle}} and object
creation/addition are not atomic. Multiple threads can pass the check
simultaneously before any add objects, exceeding {{{}maxIdle{}}}.
*Why {{maxTotal}} works but {{maxIdle}} doesn't:* The {{create()}} method
enforces {{maxTotal}} via synchronized access to {{{}createCount{}}}, but no
such protection exists for {{{}maxIdle{}}}.
h2. Reproduction
h3. Test Case 1:
{code:java}
@Test
@Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
void testAddObjectRespectsMaxIdleLimit() throws Exception {
genericObjectPool.setMaxIdle(1);
genericObjectPool.addObject();
genericObjectPool.addObject();
assertEquals(1, genericObjectPool.getNumIdle(), "should be one idle");
genericObjectPool.setMaxIdle(-1);
genericObjectPool.addObject();
genericObjectPool.addObject();
genericObjectPool.addObject();
assertEquals(4, genericObjectPool.getNumIdle(), "should be four idle");
}
{code}
h3. Test Case 2: Concurrent (Race Condition)
{code:java}
@Test
@Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
void testAddObjectConcurrentCallsRespectsMaxIdle() throws Exception {
final int maxIdleLimit = 5;
final int numThreads = 10;
genericObjectPool.setMaxIdle(maxIdleLimit);
final CountDownLatch startLatch = new CountDownLatch(1);
final CountDownLatch doneLatch = new CountDownLatch(numThreads);
List<Runnable> tasks = getRunnables(numThreads, startLatch, doneLatch);
ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
tasks.forEach(executorService::submit);
try {
startLatch.countDown(); // Start all threads simultaneously
doneLatch.await(); // Wait for all threads to complete
} finally {
executorService.shutdown();
assertTrue(executorService.awaitTermination(Long.MAX_VALUE,
TimeUnit.NANOSECONDS));
}
assertTrue(genericObjectPool.getNumIdle() <= maxIdleLimit,
"Concurrent addObject() calls should not exceed maxIdle limit of " +
maxIdleLimit +
", but found " + genericObjectPool.getNumIdle() + " idle objects");
}
private List<Runnable> getRunnables(final int numThreads,
final CountDownLatch startLatch,
final CountDownLatch doneLatch) {
List<Runnable> tasks = new ArrayList<>();
for(int i = 0; i < numThreads; i++) {
tasks.add(() -> {
try {
startLatch.await(); // Wait for all threads to be ready
genericObjectPool.addObject();
} catch (Exception e) {
Thread.currentThread().interrupt(); // Restore interrupt status
} finally {
doneLatch.countDown();
}
});
}
return tasks;
}
{code}
h2. Proposed Fix
The pool should ensure to *the minimum of these two values* as the effective
limit when deciding whether to add new objects. This ensures the pool respects
whichever limit is lower and prevents violating either configuration constraint.
See PR for the fix implementation:
[https://github.com/apache/commons-pool/pull/450]
And see this PR for running the tests :-
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
