Skip to content

Commit e2c3d1f

Browse files
psteitzpsteitz
authored
Fix regression in fix for POOL-425, fix race in POOL-426. (#452)
1 parent baa9891 commit e2c3d1f

File tree

3 files changed

+96
-6
lines changed

3 files changed

+96
-6
lines changed

‎src/changes/changes.xml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,10 @@ The <action> type attribute can be add,update,fix,remove.
4545
<title>Apache Commons Pool Release Notes</title>
4646
</properties>
4747
<body>
48-
<release version="" date="YYYY-MM-DD" description="This is a feature and maintenance release. Java 8 or later is required.">
48+
<release version="2.13.1" date="YYYY-MM-DD" description="This is a feature and maintenance release. Java 8 or later is required.">
4949
<!-- FIX -->
50+
<action type="fix" issue="POOL-427" dev="psteitz" due-to="Raju Gupta"> The fix for POOL-425 introduced a regression where addObject fails when maxIdle is negative (indicating no limit).</action>
51+
<action type="fix" issue="POOL-426" dev="psteitz" due-to="Raju Gupta">GenericObjectPool addObject can cause maxIdle to be exceeded</action>
5052
<!-- ADD -->
5153
<!-- UPDATE -->
5254
</release>

‎src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,11 +212,16 @@ public void addObject() throws Exception {
212212
if (factory == null) {
213213
throw new IllegalStateException("Cannot add objects without a factory.");
214214
}
215-
216-
final int localMaxTotal = getMaxTotal();
217-
final int localMaxIdle = getMaxIdle();
218-
if (getNumIdle() < localMaxIdle && (localMaxTotal < 0 || createCount.get() < localMaxTotal)) {
219-
addIdleObject(create(getMaxWaitDuration()));
215+
synchronized (makeObjectCountLock) {
216+
final int localMaxTotal = getMaxTotal();
217+
final int localMaxIdle = getMaxIdle();
218+
if (localMaxIdle >= 0 && getNumIdle() >= localMaxIdle) {
219+
// Pool already has maxIdle idle objects or maxIdle is 0.
220+
return;
221+
}
222+
if (localMaxTotal < 0 || createCount.get() < localMaxTotal) {
223+
addIdleObject(create(getMaxWaitDuration()));
224+
}
220225
}
221226
}
222227

‎src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3065,4 +3065,87 @@ void testWhenExhaustedFail() throws Exception {
30653065
assertEquals(1, genericObjectPool.getNumIdle());
30663066
genericObjectPool.close();
30673067
}
3068+
3069+
@Test
3070+
void testAddObjectNegativeMaxTotal() throws Exception {
3071+
genericObjectPool.setMaxTotal(-1);
3072+
genericObjectPool.addObject();
3073+
assertEquals(1, genericObjectPool.getNumIdle());
3074+
}
3075+
3076+
@Test
3077+
void testAddObjectNegativeMaxIdle() throws Exception {
3078+
genericObjectPool.setMaxIdle(-1);
3079+
genericObjectPool.addObject();
3080+
assertEquals(1, genericObjectPool.getNumIdle());
3081+
}
3082+
3083+
@Test
3084+
void testAddObjectZeroMaxIdle() throws Exception {
3085+
genericObjectPool.setMaxIdle(0);
3086+
genericObjectPool.addObject();
3087+
assertEquals(0, genericObjectPool.getNumIdle());
3088+
}
3089+
3090+
@Test
3091+
@Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
3092+
void testAddObjectRespectsMaxIdleLimit() throws Exception {
3093+
genericObjectPool.setMaxIdle(1);
3094+
genericObjectPool.addObject();
3095+
genericObjectPool.addObject();
3096+
assertEquals(1, genericObjectPool.getNumIdle(), "should be one idle");
3097+
3098+
genericObjectPool.setMaxIdle(-1);
3099+
genericObjectPool.addObject();
3100+
genericObjectPool.addObject();
3101+
genericObjectPool.addObject();
3102+
assertEquals(4, genericObjectPool.getNumIdle(), "should be four idle");
3103+
}
3104+
3105+
@Test
3106+
@Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
3107+
void testAddObjectConcurrentCallsRespectsMaxIdle() throws Exception {
3108+
final int maxIdleLimit = 5;
3109+
final int numThreads = 10;
3110+
genericObjectPool.setMaxIdle(maxIdleLimit);
3111+
3112+
final CountDownLatch startLatch = new CountDownLatch(1);
3113+
final CountDownLatch doneLatch = new CountDownLatch(numThreads);
3114+
3115+
final List<Runnable> tasks = getRunnables(numThreads, startLatch, doneLatch);
3116+
3117+
final ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
3118+
tasks.forEach(executorService::submit);
3119+
try {
3120+
startLatch.countDown(); // Start all threads simultaneously
3121+
doneLatch.await(); // Wait for all threads to complete
3122+
} finally {
3123+
executorService.shutdown();
3124+
assertTrue(executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS));
3125+
}
3126+
3127+
assertTrue(genericObjectPool.getNumIdle() <= maxIdleLimit,
3128+
"Concurrent addObject() calls should not exceed maxIdle limit of " + maxIdleLimit +
3129+
", but found " + genericObjectPool.getNumIdle() + " idle objects");
3130+
}
3131+
3132+
private List<Runnable> getRunnables(final int numThreads,
3133+
final CountDownLatch startLatch,
3134+
final CountDownLatch doneLatch) {
3135+
final List<Runnable> tasks = new ArrayList<>();
3136+
3137+
for (int i = 0; i < numThreads; i++) {
3138+
tasks.add(() -> {
3139+
try {
3140+
startLatch.await(); // Wait for all threads to be ready
3141+
genericObjectPool.addObject();
3142+
} catch (Exception e) {
3143+
Thread.currentThread().interrupt(); // Restore interrupt status
3144+
} finally {
3145+
doneLatch.countDown();
3146+
}
3147+
});
3148+
}
3149+
return tasks;
3150+
}
30683151
}

0 commit comments

Comments
 (0)