This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-pool.git
The following commit(s) were added to refs/heads/master by this push:
new 92499d61 [POOL-411] NPE when deregistering key at end of borrow
92499d61 is described below
commit 92499d6170db610a7a042e5eaa51229910acc771
Author: Gary Gregory <garydgreg...@gmail.com>
AuthorDate: Sun Mar 5 07:55:24 2023 -0500
[POOL-411] NPE when deregistering key at end of borrow
---
src/changes/changes.xml | 3 +
.../commons/pool2/impl/GenericKeyedObjectPool.java | 31 +++++-----
.../pool2/impl/TestGenericKeyedObjectPool.java | 68 +++++++++++++++++++++-
3 files changed, 85 insertions(+), 17 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 55d8477f..1d21c8d3 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -75,6 +75,9 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" type="fix" due-to="Réda Housni Alaoui, Gary
Gregory">
Null-guard in GenericObjectPool.use(T) like other call sites of
GenericObjectPool.getPooledObject(T).
</action>
+ <action dev="ggregory" type="fix" issue="POOL-411" due-to="Richard Eckart
de Castilho, Gary Gregory">
+ NPE when deregistering key at end of borrow.
+ </action>
<!-- ADD -->
<action dev="ggregory" type="add" due-to="Gary Gregory">
Add PooledObject.getFullDuration().
diff --git
a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
index 0697ef83..17fb18df 100644
--- a/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
@@ -312,7 +312,6 @@ public class GenericKeyedObjectPool<K, T, E extends
Exception> extends BaseGener
}
}
-
/**
* Create an object using the {@link KeyedPooledObjectFactory#makeObject
* factory}, passivate it, and then place it in the idle object pool.
@@ -335,7 +334,6 @@ public class GenericKeyedObjectPool<K, T, E extends
Exception> extends BaseGener
}
}
-
/**
* Equivalent to <code>{@link #borrowObject(Object, long)
borrowObject}(key,
* {@link #getMaxWaitDuration()})</code>.
@@ -347,7 +345,6 @@ public class GenericKeyedObjectPool<K, T, E extends
Exception> extends BaseGener
return borrowObject(key, getMaxWaitDuration().toMillis());
}
-
/**
* Borrows an object from the sub-pool associated with the given key using
* the specified waiting time which only applies if
@@ -834,19 +831,21 @@ public class GenericKeyedObjectPool<K, T, E extends
Exception> extends BaseGener
try {
lock.lock();
final ObjectDeque<T> objectDeque = poolMap.get(k);
- final long numInterested =
objectDeque.getNumInterested().decrementAndGet();
- if (numInterested == 0 && objectDeque.getCreateCount().get() == 0)
{
- // Potential to remove key
- // Upgrade to write lock
- lock.unlock();
- lock = keyLock.writeLock();
- lock.lock();
- if (objectDeque.getCreateCount().get() == 0 &&
objectDeque.getNumInterested().get() == 0) {
- // NOTE: Keys must always be removed from both poolMap and
- // poolKeyList at the same time while protected by
- // keyLock.writeLock()
- poolMap.remove(k);
- poolKeyList.remove(k);
+ if (objectDeque != null) {
+ final long numInterested =
objectDeque.getNumInterested().decrementAndGet();
+ if (numInterested == 0 && objectDeque.getCreateCount().get()
== 0) {
+ // Potential to remove key
+ // Upgrade to write lock
+ lock.unlock();
+ lock = keyLock.writeLock();
+ lock.lock();
+ if (objectDeque.getCreateCount().get() == 0 &&
objectDeque.getNumInterested().get() == 0) {
+ // NOTE: Keys must always be removed from both poolMap
and
+ // poolKeyList at the same time while protected by
+ // keyLock.writeLock()
+ poolMap.remove(k);
+ poolKeyList.remove(k);
+ }
}
}
} finally {
diff --git
a/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
b/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
index 9ec3f60a..3994c219 100644
---
a/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
+++
b/src/test/java/org/apache/commons/pool2/impl/TestGenericKeyedObjectPool.java
@@ -33,7 +33,9 @@ import java.io.StringWriter;
import java.lang.management.ManagementFactory;
import java.time.Duration;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashSet;
+import java.util.List;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.Random;
@@ -41,6 +43,7 @@ import java.util.Set;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
@@ -976,7 +979,7 @@ public class TestGenericKeyedObjectPool extends
TestKeyedObjectPool {
// Start and park threads waiting to borrow objects
final TestThread[] threads = new TestThread[numThreads];
- for(int i=0;i<numThreads;i++) {
+ for (int i = 0; i < numThreads; i++) {
threads[i] = new TestThread<>(gkoPool, 1, 0, 2000, false, "0" +
String.valueOf(i % maxTotal), "0");
final Thread t = new Thread(threads[i]);
t.start();
@@ -1004,6 +1007,69 @@ public class TestGenericKeyedObjectPool extends
TestKeyedObjectPool {
}
}
+ /**
+ * TODO Tests POOL-411, or least tries to reproduce the NPE, but does not.
+ *
+ * @throws TestException a test failure.
+ */
+ @Test
+ public void testConcurrentBorrowAndClear() throws TestException {
+ final int threadCount = 64;
+ final int taskCount = 64;
+ final int addCount = 1;
+ final int borrowCycles = 1024;
+ final int clearCycles = 1024;
+
+ final GenericKeyedObjectPoolConfig<String> config = new
GenericKeyedObjectPoolConfig<>();
+ final int maxTotalPerKey = borrowCycles + 1;
+ config.setMaxTotalPerKey(threadCount);
+ config.setMaxIdlePerKey(threadCount);
+ config.setMaxTotal(maxTotalPerKey * threadCount);
+ config.setBlockWhenExhausted(false); // pool exhausted indicates a bug
in the test
+
+ gkoPool = new GenericKeyedObjectPool<>(simpleFactory, config);
+ gkoPool.addObjects(Arrays.asList("0"), threadCount);
+ // all objects in the pool are now idle.
+
+ final ExecutorService threadPool =
Executors.newFixedThreadPool(threadCount);
+ final List<Future<?>> futures = new ArrayList<>();
+ try {
+ for (int t = 0; t < taskCount; t++) {
+ futures.add(threadPool.submit(() -> {
+ for (int i = 0; i < clearCycles; i++) {
+ Thread.yield();
+ gkoPool.clear("0", true);
+ try {
+ gkoPool.addObjects(Arrays.asList("0"), addCount);
+ } catch (IllegalArgumentException | TestException e) {
+ fail(e);
+ }
+ }
+ }));
+ futures.add(threadPool.submit(() -> {
+ try {
+ for (int i = 0; i < borrowCycles; i++) {
+ Thread.yield();
+ final String pooled = gkoPool.borrowObject("0");
+ gkoPool.returnObject("0", pooled);
+ }
+ } catch (TestException e) {
+ fail(e);
+ }
+ }));
+ }
+ futures.forEach(f -> {
+ try {
+ f.get();
+ } catch (InterruptedException | ExecutionException e) {
+ fail(e);
+ }
+ });
+ } finally {
+ threadPool.shutdownNow();
+ }
+ }
+
/**
* POOL-192
* Verify that clear(key) does not leak capacity.
