[
https://issues.apache.org/jira/browse/POOL-426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18047514#comment-18047514
]
Raju Gupta edited comment on POOL-426 at 12/25/25 12:11 PM:
------------------------------------------------------------
[~chadwilson] Think this might be related. I have added a test for the same at
[here| [https://github.com/apache/commons-pool/pull/451
|https://github.com/apache/commons-pool/pull/451.]] Let me know if this works
for you.
was (Author: JIRAUSER308107):
[~chadwilson] Think this might be related. I have added a test for the same at
[here|[https://github.com/apache/commons-pool/pull/451|https://github.com/apache/commons-pool/pull/451.]]
Let me know if this works for you.
> GenericObjectPool does not respect maxIdle 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)
