[
https://issues.apache.org/jira/browse/POOL-426?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18047522#comment-18047522
]
Chad Wilson commented on POOL-426:
----------------------------------
I didn't report that issue, which is from 2 years ago. Probably better to
interact with the OP there.
I was just searching for possible reasons for a regression Im seeing in 2.13.0
with a pool or connection not being closed properly in some circumstance that
used to work on 2.12.2. Came across these two which seem to basically be
discussing the same code as each other, describing a similar check-then-act
race condition - although probably not the root cause of my problem in the end.
> 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)
