Skip to content

Commit 32fd701

Browse files
psteitzpsteitz
committed
Ensure that invalidateObject replaces destroyed instance. JIRA: POOL-424
1 parent c5d63ad commit 32fd701

File tree

6 files changed

+75
-10
lines changed

6 files changed

+75
-10
lines changed

‎src/changes/changes.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ The <action> type attribute can be add,update,fix,remove.
4747
<body>
4848
<release version="2.13.0" 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-424" dev="psteitz" due-to="Steven Adams">GenericObjectPool.invalidateObject() can leave other threads waiting to borrow hanging.
51+
The fix for this issue changes behavior of invalidateObject. This method now always tries to add a new instance
52+
to the pool to replace the invalidated and destroyed instance. As a result of this change, abandoned object
53+
removal now attemps to replace abandoned objects.
54+
</action>
5055
<action type="fix" issue="POOL-425" dev="psteitz">GenericObjectPool addObject does not respect maxIdle.</action>
5156
<action type="fix" issue="POOL-350" dev="psteitz">Make placement of calls to GKOP reuseCapacity configurable.</action>
5257
<action type="fix" issue="POOL-290" dev="psteitz" due-to="Serge Angelov">TestSoftRefOutOfMemory (unit test) can loop infinitely on failure.</action>

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -913,7 +913,8 @@ public void invalidateObject(final T obj) throws Exception {
913913
* {@inheritDoc}
914914
* <p>
915915
* Activation of this method decrements the active count and attempts to destroy the instance, using the provided
916-
* {@link DestroyMode}.
916+
* {@link DestroyMode}. To ensure liveness of the pool, {@link #addObject()} is called to replace the invalidated
917+
* instance.
917918
* </p>
918919
*
919920
* @throws Exception if an exception occurs destroying the object
@@ -934,7 +935,9 @@ public void invalidateObject(final T obj, final DestroyMode destroyMode) throws
934935
destroy(p, destroyMode);
935936
}
936937
}
937-
ensureIdle(1, false);
938+
if (!isClosed()) {
939+
addObject();
940+
}
938941
}
939942

940943
/**
@@ -970,7 +973,10 @@ public void preparePool() throws Exception {
970973

971974
/**
972975
* Recovers abandoned objects which have been checked out but
973-
* not used since longer than the removeAbandonedTimeout.
976+
* not used since longer than the removeAbandonedTimeout. For each object
977+
* deemed abandoned, {@link #invalidateObject(Object)} is called. This
978+
* results in the object being destroyed and then {@link #addObject()} is
979+
* called to try to replace it.
974980
*
975981
* @param abandonedConfig The configuration to use to identify abandoned objects
976982
*/

‎src/test/java/org/apache/commons/pool2/AbstractTestObjectPool.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,6 @@ void testClosedPoolBehavior() throws Exception {
101101
assertEquals(1, pool.getNumActive(), "A closed pool should still keep count of active objects.");
102102
}
103103
pool.invalidateObject(o2);
104-
if (pool.getNumIdle() >= 0) {
105-
assertEquals(0, pool.getNumIdle(), "invalidateObject must not add items back into the idle object pool.");
106-
}
107104
if (pool.getNumActive() >= 0) {
108105
assertEquals(0, pool.getNumActive(), "A closed pool should still keep count of active objects.");
109106
}
@@ -298,7 +295,7 @@ void testPOFInvalidateObjectUsages() throws Exception {
298295
// invalidated object should be destroyed
299296
pool.invalidateObject(obj);
300297
expectedMethods.add(new MethodCall("destroyObject", obj));
301-
assertEquals(expectedMethods, factory.getMethodCalls());
298+
assertTrue(factory.getMethodCalls().containsAll(expectedMethods));
302299
// Test exception handling of invalidateObject
303300
reset(pool, factory, expectedMethods);
304301
final Object obj2 = pool.borrowObject();

‎src/test/java/org/apache/commons/pool2/TestBaseObjectPool.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,8 @@ void testBaseInvalidateObject() throws Exception {
224224
assertEquals(0, pool.getNumIdle());
225225
pool.invalidateObject(obj0);
226226
assertEquals(1, pool.getNumActive());
227-
assertEquals(0, pool.getNumIdle());
228227
pool.invalidateObject(obj1);
229228
assertEquals(0, pool.getNumActive());
230-
assertEquals(0, pool.getNumIdle());
231229
pool.close();
232230
}
233231

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,10 @@ void testAbandonedReturn() throws Exception {
333333
// will deem abandoned. Make sure it is not returned to the borrower.
334334
returner.start(); // short delay, then return instance
335335
assertTrue(pool.borrowObject().hashCode() != deadMansHash);
336-
assertEquals(0, pool.getNumIdle());
336+
// There should be n - 2 - 1 idle instance in the pool
337+
// The n - 2 instances deemed abandoned should have been replaced, but
338+
// one of the new ones has been checked out.
339+
assertEquals(n - 2 - 1, pool.getNumIdle());
337340
assertEquals(1, pool.getNumActive());
338341
}
339342

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3009,4 +3009,60 @@ void testAddObjectCanAddToMaxIdle() throws Exception {
30093009
}
30103010
assertEquals(3, genericObjectPool.getNumIdle());
30113011
}
3012+
3013+
/**
3014+
* Verify that when thread A borrows two objects from a pool with maxTotal=2,
3015+
* then two more threads try to borrow, returning one object and invalidating
3016+
* the other will serve both of the waiting threads.
3017+
*/
3018+
@Test
3019+
@Timeout(value = 10_000, unit = TimeUnit.MILLISECONDS)
3020+
void testReturnAndInvalidateServesWaiters() throws Exception {
3021+
genericObjectPool.setMaxTotal(2);
3022+
genericObjectPool.setBlockWhenExhausted(true);
3023+
genericObjectPool.setMaxWait(Duration.ofMillis(100));
3024+
3025+
// Thread A borrows two objects
3026+
final String obj1 = genericObjectPool.borrowObject();
3027+
final String obj2 = genericObjectPool.borrowObject();
3028+
3029+
// Track successful borrows
3030+
final AtomicInteger successfulBorrows = new AtomicInteger(0);
3031+
3032+
// Create two borrowing threads that will block
3033+
final Thread borrower1 = new Thread(() -> {
3034+
try {
3035+
genericObjectPool.borrowObject();
3036+
successfulBorrows.incrementAndGet();
3037+
} catch (final Exception e) {
3038+
// Borrow failed
3039+
}
3040+
});
3041+
3042+
final Thread borrower2 = new Thread(() -> {
3043+
try {
3044+
genericObjectPool.borrowObject();
3045+
successfulBorrows.incrementAndGet();
3046+
} catch (final Exception e) {
3047+
// Borrow failed
3048+
}
3049+
});
3050+
3051+
// Start the borrowing threads - they will block
3052+
borrower1.start();
3053+
borrower2.start();
3054+
Thread.sleep(50); // Give threads time to start and block
3055+
3056+
// Thread A returns one object and invalidates the other with no delay
3057+
genericObjectPool.invalidateObject(obj1);
3058+
genericObjectPool.invalidateObject(obj2);
3059+
3060+
// Wait for threads to complete
3061+
borrower1.join();
3062+
borrower2.join();
3063+
3064+
// Both threads should have been served
3065+
assertEquals(2, successfulBorrows.get(),
3066+
"Both waiting threads should have been served, but only " + successfulBorrows.get() + " were served");
3067+
}
30123068
}

0 commit comments

Comments
 (0)