You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Raju Gupta (Jira)" <ji...@apache.org> on 2025/12/14 21:25:00 UTC
[jira] [Updated] (POOL-426) GenericObjectPool does not respect minIdle configuration
[ https://issues.apache.org/jira/browse/POOL-426?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Raju Gupta updated POOL-426:
----------------------------
Description:
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 :- https://github.com/apache/commons-pool/pull/451
was:
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 :-
> 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
> Priority: Critical
>
> 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 :- https://github.com/apache/commons-pool/pull/451
--
This message was sent by Atlassian Jira
(v8.20.10#820010)